-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[SPIRV] Add FPEncoding operand support for OpTypeFloat #156871
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-backend-spir-v Author: None (YixingZhang007) ChangesFull diff: https://github.com/llvm/llvm-project/pull/156871.diff 2 Files Affected:
diff --git a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
index cfe24c84941a9..0f258e03b23c8 100644
--- a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
@@ -1122,7 +1122,19 @@ SPIRVType *SPIRVGlobalRegistry::restOfCreateSPIRVType(
SPIRVType *SpirvType = createSPIRVType(Ty, MIRBuilder, AccessQual,
ExplicitLayoutRequired, EmitIR);
TypesInProcessing.erase(Ty);
- VRegToTypeMap[&MIRBuilder.getMF()][getSPIRVTypeID(SpirvType)] = SpirvType;
+
+ // Record the FPVariant of the floating-point registers in the
+ // VRegFPVariantMap.
+ MachineFunction *MF = &MIRBuilder.getMF();
+ Register TypeReg = getSPIRVTypeID(SpirvType);
+ if (Ty->isFloatingPointTy()) {
+ if (Ty->isBFloatTy()) {
+ VRegFPVariantMap[MF][TypeReg] = FPVariant::BRAIN_FLOAT;
+ } else {
+ VRegFPVariantMap[MF][TypeReg] = FPVariant::IEEE_FLOAT;
+ }
+ }
+ VRegToTypeMap[MF][TypeReg] = SpirvType;
// TODO: We could end up with two SPIR-V types pointing to the same llvm type.
// Is that a problem?
@@ -2088,3 +2100,15 @@ bool SPIRVGlobalRegistry::hasBlockDecoration(SPIRVType *Type) const {
}
return false;
}
+
+SPIRVGlobalRegistry::FPVariant
+SPIRVGlobalRegistry::getFPVariantForVReg(Register VReg,
+ const MachineFunction *MF) {
+ auto t = VRegFPVariantMap.find(MF ? MF : CurMF);
+ if (t != VRegFPVariantMap.end()) {
+ auto tt = t->second.find(VReg);
+ if (tt != t->second.end())
+ return tt->second;
+ }
+ return FPVariant::NONE;
+}
\ No newline at end of file
diff --git a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
index 7ef812828b7cc..1f8c30dc01f7f 100644
--- a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
+++ b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
@@ -29,6 +29,10 @@ using SPIRVType = const MachineInstr;
using StructOffsetDecorator = std::function<void(Register)>;
class SPIRVGlobalRegistry : public SPIRVIRMapping {
+public:
+ enum class FPVariant { NONE, IEEE_FLOAT, BRAIN_FLOAT };
+
+private:
// Registers holding values which have types associated with them.
// Initialized upon VReg definition in IRTranslator.
// Do not confuse this with DuplicatesTracker as DT maps Type* to <MF, Reg>
@@ -88,6 +92,11 @@ class SPIRVGlobalRegistry : public SPIRVIRMapping {
// map of aliasing decorations to aliasing metadata
std::unordered_map<const MDNode *, MachineInstr *> AliasInstMDMap;
+ // Maps floating point Registers to their FPVariant (float type kind), given
+ // the MachineFunction.
+ DenseMap<const MachineFunction *, DenseMap<Register, FPVariant>>
+ VRegFPVariantMap;
+
// Add a new OpTypeXXX instruction without checking for duplicates.
SPIRVType *createSPIRVType(const Type *Type, MachineIRBuilder &MIRBuilder,
SPIRV::AccessQualifier::AccessQualifier AQ,
@@ -422,6 +431,10 @@ class SPIRVGlobalRegistry : public SPIRVIRMapping {
// structures referring this instruction.
void invalidateMachineInstr(MachineInstr *MI);
+ // Return the FPVariant of to the given floating-point regiester.
+ FPVariant getFPVariantForVReg(Register VReg,
+ const MachineFunction *MF = nullptr);
+
private:
SPIRVType *getOpTypeBool(MachineIRBuilder &MIRBuilder);
|
This PR is a prerequisite for adding support for the SPIR-V extension SPV_KHR_bfloat16 in PR #155645 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of nits, but looks good to me otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto t = VRegFPVariantMap.find(MF ? MF : CurMF); | |
auto T = VRegFPVariantMap.find(MF ? MF : CurMF); |
Also, can we have the explicit type if it's not too complicated? Makes the code easier to read. I wouldn't mind a more descriptive variable name either :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! I have made the change :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto tt = t->second.find(VReg); | |
auto tt = t->second.find(VReg); |
Same than before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure! I have fixed it. Thanks!
✅ With the latest revision this PR passed the C/C++ code formatter. |
7459a65
to
a67050e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize it was such a nasty type. You can go back to auto if you wish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree auto
might be a better choice here. I’ve updated both iterator types to use auto
. Thanks for the suggestion :)
The current CI failure in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we make it unreachable instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could happen if the register is not a floating pointer register right? I think we still want want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! I’ve updated the approach for representing floating-point types in the PR, and FPVariant
is no longer used. Please let me know if there’s anything I can improve with the current approach. Thank you! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it's not enough to use VRegToTypeMap? Object that holds OpTypeFloat 16
should be different from OpTypeFloat 16 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry that there was a mistake in my initial thinking. I thought OpTypeFloat
was a non-variadic SPIR-V operation, so all floating points would be represented with instruction OpTypeFloat 16
. I have updated OpTypeFloat
to now be a variadic operation. Thanks :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any problems with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could happen if the register is not a floating pointer register right? I think we still want want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues with this, but seconding the question about the reuse of the existing VRegToTypeMap
. Is there a performance need to store the FPVariant in a map, or can getFPVariantForVreg do a lookup in the generic map and then do the isBFloatTy
/isFloatTy
every time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Return the FPVariant of to the given floating-point regiester. | |
// Return the FPVariant of to the given floating-point register. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! I’ve updated the approach for representing floating-point types in the PR. FPVariant
and VRegFPVariantMap
are no longer used. Instead, we introduced an FPEncoding
enum class, which is now placed as the second operand of OpTypeFloat
SPIR-V instruction. Please feel free to let me know if there’s anything I can improve with the current approach. Thank you! :)
I’ve updated this PR by changing the approach for tracking the type of floating-point registers (IEEE 754 vs. Bfloat). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Should we add any test to see if we can properly generate bfloat16 type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Thanks for the suggestion! I have added test case for bfloat in the SPIR-V test |
ac5fc5d
to
68c8bbd
Compare
4a8d961
to
5b8b92d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test failure is unrelated and will be handled separately.
This PR introduces support for
FPEncoding
operand for SPIR-V instructionOpTypeFloat
, with the following main changes:FPEncoding
enum class to represent floating-point encodings, such asBFloat16KHR
, in SPIR-V.OpTypeFloat
to acceptFPEncoding
as its second input operand.This PR enables support for the BFloat floating-point type in SPIR-V.