-
Notifications
You must be signed in to change notification settings - Fork 15k
[HLSL] Adding DXIL Storage type into TypedInfo
#164887
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-llvm-analysis @llvm/pr-subscribers-backend-directx Author: None (joaosaffran) ChangesIn DXIL, some 64bit types are actually represented with their 32bit counterpart. This was already being address in the codegen, however the metadata generation was lacking this information. This PR is fixing this issue. Closes: #146735 Full diff: https://github.com/llvm/llvm-project/pull/164887.diff 5 Files Affected:
diff --git a/llvm/include/llvm/Analysis/DXILResource.h b/llvm/include/llvm/Analysis/DXILResource.h
index c7aff167324e6..6365280be806a 100644
--- a/llvm/include/llvm/Analysis/DXILResource.h
+++ b/llvm/include/llvm/Analysis/DXILResource.h
@@ -293,6 +293,8 @@ class ResourceTypeInfo {
struct TypedInfo {
dxil::ElementType ElementTy;
+ // Some 64 byte types are treated as 32 byte types in DXIL.
+ dxil::ElementType DXILTargetTy;
uint32_t ElementCount;
bool operator==(const TypedInfo &RHS) const {
diff --git a/llvm/lib/Analysis/DXILResource.cpp b/llvm/lib/Analysis/DXILResource.cpp
index 6f19a68dcd194..b67c2c8e828ec 100644
--- a/llvm/lib/Analysis/DXILResource.cpp
+++ b/llvm/lib/Analysis/DXILResource.cpp
@@ -206,6 +206,14 @@ static dxil::ElementType toDXILElementType(Type *Ty, bool IsSigned) {
return ElementType::Invalid;
}
+static dxil::ElementType toDXILTargetType(Type *Ty, bool IsSigned) {
+ // TODO: Handle unorm, snorm, and packed.
+ Type *ScalarTy = Ty->getScalarType();
+ if (ScalarTy->isIntegerTy(64) || ScalarTy->isDoubleTy())
+ return ElementType::U32;
+ return toDXILElementType(Ty, IsSigned);
+}
+
ResourceTypeInfo::ResourceTypeInfo(TargetExtType *HandleTy,
const dxil::ResourceClass RC_,
const dxil::ResourceKind Kind_)
@@ -569,10 +577,11 @@ ResourceTypeInfo::TypedInfo ResourceTypeInfo::getTyped() const {
auto [ElTy, IsSigned] = getTypedElementType(Kind, HandleTy);
dxil::ElementType ET = toDXILElementType(ElTy, IsSigned);
+ dxil::ElementType DXILTargetTy = toDXILTargetType(ElTy, IsSigned);
uint32_t Count = 1;
if (auto *VTy = dyn_cast<FixedVectorType>(ElTy))
Count = VTy->getNumElements();
- return {ET, Count};
+ return {ET, DXILTargetTy, Count};
}
dxil::SamplerFeedbackType ResourceTypeInfo::getFeedbackType() const {
@@ -714,7 +723,8 @@ MDTuple *ResourceInfo::getAsMetadata(Module &M,
Tags.push_back(getIntMD(RTI.getStruct(DL).Stride));
} else if (RTI.isTyped()) {
Tags.push_back(getIntMD(llvm::to_underlying(ExtPropTags::ElementType)));
- Tags.push_back(getIntMD(llvm::to_underlying(RTI.getTyped().ElementTy)));
+ Tags.push_back(
+ getIntMD(llvm::to_underlying(RTI.getTyped().DXILTargetTy)));
} else if (RTI.isFeedback()) {
Tags.push_back(
getIntMD(llvm::to_underlying(ExtPropTags::SamplerFeedbackKind)));
diff --git a/llvm/lib/Target/DirectX/DXILPrettyPrinter.cpp b/llvm/lib/Target/DirectX/DXILPrettyPrinter.cpp
index dc84ae49abbe3..56fcae19f59e3 100644
--- a/llvm/lib/Target/DirectX/DXILPrettyPrinter.cpp
+++ b/llvm/lib/Target/DirectX/DXILPrettyPrinter.cpp
@@ -49,7 +49,7 @@ static StringRef getRCPrefix(dxil::ResourceClass RC) {
static StringRef getFormatName(const dxil::ResourceTypeInfo &RI) {
if (RI.isTyped()) {
- switch (RI.getTyped().ElementTy) {
+ switch (RI.getTyped().DXILTargetTy) {
case dxil::ElementType::I1:
return "i1";
case dxil::ElementType::I16:
diff --git a/llvm/test/CodeGen/DirectX/Metadata/srv_metadata.ll b/llvm/test/CodeGen/DirectX/Metadata/srv_metadata.ll
index a2059beeb0acb..0062f90326490 100644
--- a/llvm/test/CodeGen/DirectX/Metadata/srv_metadata.ll
+++ b/llvm/test/CodeGen/DirectX/Metadata/srv_metadata.ll
@@ -22,14 +22,14 @@ target triple = "dxil-pc-shadermodel6.6-compute"
; PRINT-NEXT:; ------------------------------ ---------- ------- ----------- ------- -------------- ---------
; PRINT-NEXT:; Zero texture f16 buf T0 t0 1
; PRINT-NEXT:; One texture f32 buf T1 t1 1
-; PRINT-NEXT:; Two texture f64 buf T2 t2 1
+; PRINT-NEXT:; Two texture u32 buf T2 t2 1
; PRINT-NEXT:; Three texture i32 buf T3 t3 1
; PRINT-NEXT:; Four texture byte r/o T4 t5 1
; PRINT-NEXT:; Five texture struct r/o T5 t6 1
-; PRINT-NEXT:; Six texture u64 buf T6 t10,space2 1
+; PRINT-NEXT:; Six texture u32 buf T6 t10,space2 1
; PRINT-NEXT:; Array texture f32 buf T7 t4,space3 100
-; PRINT-NEXT:; Array2 texture f64 buf T8 t2,space4 unbounded
-; PRINT-NEXT:; Seven texture u64 buf T9 t20,space5 1
+; PRINT-NEXT:; Array2 texture u32 buf T8 t2,space4 unbounded
+; PRINT-NEXT:; Seven texture u32 buf T9 t20,space5 1
;
define void @test() #0 {
@@ -120,15 +120,14 @@ attributes #0 = { noinline nounwind "hlsl.shader"="compute" }
; CHECK: ![[Half]] = !{i32 0, i32 8}
; CHECK: ![[One]] = !{i32 1, ptr @One, !"One", i32 0, i32 1, i32 1, i32 10, i32 0, ![[Float:[0-9]+]]}
; CHECK: ![[Float]] = !{i32 0, i32 9}
-; CHECK: ![[Two]] = !{i32 2, ptr @Two, !"Two", i32 0, i32 2, i32 1, i32 10, i32 0, ![[Double:[0-9]+]]}
-; CHECK: ![[Double]] = !{i32 0, i32 10}
+; CHECK: ![[Two]] = !{i32 2, ptr @Two, !"Two", i32 0, i32 2, i32 1, i32 10, i32 0, ![[U32:[0-9]+]]}
+; CHECK: ![[U32]] = !{i32 0, i32 5}
; CHECK: ![[Three]] = !{i32 3, ptr @Three, !"Three", i32 0, i32 3, i32 1, i32 10, i32 0, ![[I32:[0-9]+]]}
; CHECK: ![[I32]] = !{i32 0, i32 4}
; CHECK: ![[Four]] = !{i32 4, ptr @Four, !"Four", i32 0, i32 5, i32 1, i32 11, i32 0, null}
; CHECK: ![[Five]] = !{i32 5, ptr @Five, !"Five", i32 0, i32 6, i32 1, i32 12, i32 0, ![[FiveStride:[0-9]+]]}
; CHECK: ![[FiveStride]] = !{i32 1, i32 2}
-; CHECK: ![[Six]] = !{i32 6, ptr @Six, !"Six", i32 2, i32 10, i32 1, i32 10, i32 0, ![[U64:[0-9]+]]}
-; CHECK: ![[U64]] = !{i32 0, i32 7}
+; CHECK: ![[Six]] = !{i32 6, ptr @Six, !"Six", i32 2, i32 10, i32 1, i32 10, i32 0, ![[U32:[0-9]+]]}
; CHECK: ![[Array]] = !{i32 7, ptr @Array, !"Array", i32 3, i32 4, i32 100, i32 10, i32 0, ![[Float]]}
-; CHECK: ![[Array2]] = !{i32 8, ptr @Array2, !"Array2", i32 4, i32 2, i32 -1, i32 10, i32 0, ![[Double]]}
-; CHECK: ![[Seven]] = !{i32 9, ptr @Seven, !"Seven", i32 5, i32 20, i32 1, i32 10, i32 0, ![[U64]]}
+; CHECK: ![[Array2]] = !{i32 8, ptr @Array2, !"Array2", i32 4, i32 2, i32 -1, i32 10, i32 0, ![[U32]]}
+; CHECK: ![[Seven]] = !{i32 9, ptr @Seven, !"Seven", i32 5, i32 20, i32 1, i32 10, i32 0, ![[U32]]}
diff --git a/llvm/test/CodeGen/DirectX/Metadata/uav_metadata.ll b/llvm/test/CodeGen/DirectX/Metadata/uav_metadata.ll
index 5b2b3ef280626..d377a528abca1 100644
--- a/llvm/test/CodeGen/DirectX/Metadata/uav_metadata.ll
+++ b/llvm/test/CodeGen/DirectX/Metadata/uav_metadata.ll
@@ -25,17 +25,17 @@ target triple = "dxil-pc-shadermodel6.6-compute"
; PRINT-NEXT:; ------------------------------ ---------- ------- ----------- ------- -------------- ---------
; PRINT-NEXT:; Zero UAV f16 buf U0 u0 1
; PRINT-NEXT:; One UAV f32 buf U1 u1 1
-; PRINT-NEXT:; Two UAV f64 buf U2 u2 1
+; PRINT-NEXT:; Two UAV u32 buf U2 u2 1
; PRINT-NEXT:; Three UAV i32 buf U3 u3 1
; PRINT-NEXT:; Four UAV byte r/w U4 u5 1
; PRINT-NEXT:; Five UAV struct r/w U5 u6 1
; PRINT-NEXT:; Six UAV i32 buf U6 u7 1
; PRINT-NEXT:; Seven UAV struct r/w U7 u8 1
; PRINT-NEXT:; Eight UAV byte r/w U8 u9 1
-; PRINT-NEXT:; Nine UAV u64 buf U9 u10,space2 1
+; PRINT-NEXT:; Nine UAV u32 buf U9 u10,space2 1
; PRINT-NEXT:; Array UAV f32 buf U10 u4,space3 100
-; PRINT-NEXT:; Array2 UAV f64 buf U11 u2,space4 unbounded
-; PRINT-NEXT:; Ten UAV u64 buf U12 u22,space5 1
+; PRINT-NEXT:; Array2 UAV u32 buf U11 u2,space4 unbounded
+; PRINT-NEXT:; Ten UAV u32 buf U12 u22,space5 1
define void @test() #0 {
; RWBuffer<half4> Zero : register(u0)
@@ -144,8 +144,8 @@ attributes #0 = { noinline nounwind "hlsl.shader"="compute" }
; CHECK: ![[Half]] = !{i32 0, i32 8}
; CHECK: ![[One]] = !{i32 1, ptr @One, !"One", i32 0, i32 1, i32 1, i32 10, i1 false, i1 false, i1 false, ![[Float:[0-9]+]]}
; CHECK: ![[Float]] = !{i32 0, i32 9}
-; CHECK: ![[Two]] = !{i32 2, ptr @Two, !"Two", i32 0, i32 2, i32 1, i32 10, i1 false, i1 false, i1 false, ![[Double:[0-9]+]]}
-; CHECK: ![[Double]] = !{i32 0, i32 10}
+; CHECK: ![[Two]] = !{i32 2, ptr @Two, !"Two", i32 0, i32 2, i32 1, i32 10, i1 false, i1 false, i1 false, ![[U32:[0-9]+]]}
+; CHECK: ![[U32]] = !{i32 0, i32 5}
; CHECK: ![[Three]] = !{i32 3, ptr @Three, !"Three", i32 0, i32 3, i32 1, i32 10, i1 false, i1 false, i1 false, ![[I32:[0-9]+]]}
; CHECK: ![[I32]] = !{i32 0, i32 4}
; CHECK: ![[Four]] = !{i32 4, ptr @Four, !"Four", i32 0, i32 5, i32 1, i32 11, i1 false, i1 false, i1 false, null}
@@ -155,8 +155,7 @@ attributes #0 = { noinline nounwind "hlsl.shader"="compute" }
; CHECK: ![[Seven]] = !{i32 7, ptr @Seven, !"Seven", i32 0, i32 8, i32 1, i32 12, i1 false, i1 false, i1 true, ![[SevenStride:[0-9]+]]}
; CHECK: ![[SevenStride]] = !{i32 1, i32 16}
; CHECK: ![[Eight]] = !{i32 8, ptr @Eight, !"Eight", i32 0, i32 9, i32 1, i32 11, i1 false, i1 false, i1 true, null}
-; CHECK: ![[Nine]] = !{i32 9, ptr @Nine, !"Nine", i32 2, i32 10, i32 1, i32 10, i1 false, i1 false, i1 false, ![[U64:[0-9]+]]}
-; CHECK: ![[U64]] = !{i32 0, i32 7}
+; CHECK: ![[Nine]] = !{i32 9, ptr @Nine, !"Nine", i32 2, i32 10, i32 1, i32 10, i1 false, i1 false, i1 false, ![[U32]]}
; CHECK: ![[Array]] = !{i32 10, ptr @Array, !"Array", i32 3, i32 4, i32 100, i32 10, i1 false, i1 false, i1 false, ![[Float]]}
-; CHECK: ![[Array2]] = !{i32 11, ptr @Array2, !"Array2", i32 4, i32 2, i32 -1, i32 10, i1 false, i1 false, i1 false, ![[Double]]}
+; CHECK: ![[Array2]] = !{i32 11, ptr @Array2, !"Array2", i32 4, i32 2, i32 -1, i32 10, i1 false, i1 false, i1 false, ![[U32]]}
; CHECK: ![[Ten]] = !{i32 12, ptr @Ten, !"Ten", i32 5, i32 22, i32 1, i32 10, i1 false, i1 false, i1 false, ![[U64:[0-9]+]]}
|
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.
Please also update ResourceTypeInfo::print. We can probably make it so that it only prints if different from the ElementTy
llvm/lib/Analysis/DXILResource.cpp
Outdated
| static dxil::ElementType toDXILTargetType(Type *Ty, bool IsSigned) { | ||
| // TODO: Handle unorm, snorm, and packed. | ||
| Type *ScalarTy = Ty->getScalarType(); | ||
| if (ScalarTy->isIntegerTy(64) || ScalarTy->isDoubleTy()) | ||
| return ElementType::U32; | ||
| return toDXILElementType(Ty, IsSigned); | ||
| } |
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.
Would it be better to operate on a dxil::ElementType here rather than redo the toDXILElementType work? The only user of this has already called toDXILElementType().
|
|
||
| struct TypedInfo { | ||
| dxil::ElementType ElementTy; | ||
| // Some 64 byte types are treated as 32 byte types in DXIL. |
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 think this comment adds a lot here - it would make more sense in the toDXILTargetType function, but that function is pretty self explanatory as is.
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 second @bogner's comments, otherwise LGTM!
llvm/lib/Analysis/DXILResource.cpp
Outdated
| } | ||
|
|
||
| static dxil::ElementType toDXILTargetType(dxil::ElementType ET) { | ||
| // TODO: Handle unorm, snorm, and packed. |
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.
Packed isn't relevant here - this function is only ever interesting for 64 bit types.
llvm/lib/Analysis/DXILResource.cpp
Outdated
|
|
||
| static dxil::ElementType toDXILTargetType(dxil::ElementType ET) { | ||
| // TODO: Handle unorm, snorm, and packed. | ||
| if (ET == dxil::ElementType::U64 || ET == dxil::ElementType::F64) |
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.
What about dxil::ElementType::I64?
We could arguably pre-emptively put SNormF64 and UNormF64 in here too (They also use U32 - https://hlsl.godbolt.org/z/3jr3qG17q), but I don't think we can really test that so it's probably fine to leave it as a TODO until the toDXILElementType TODO is completed.
|
|
||
| struct TypedInfo { | ||
| dxil::ElementType ElementTy; | ||
| dxil::ElementType DXILTargetTy; |
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.
Can we come up with a better name for this? I think DXILTargetTy is a little ambiguous. This is the type we need to access the object as in resource operations, so maybe "StorageTy" or "DXILAccessTy"? Do you have any other ideas?
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!
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.
Looks good once my last couple of minor comments are addressed.
llvm/lib/Analysis/DXILResource.cpp
Outdated
| OS << " Element Type: " << getElementTypeName(Typed.ElementTy) << "\n" | ||
| << " Storage Type: " << getElementTypeName(Typed.DXILStorageTy) | ||
| << "\n" |
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 think it'd be nicer for readability if we only include this when it's different, ie:
OS << " Element Type: " << getElementTypeName(Typed.ElementTy);
if (Typed.ElementTy != Typed.DXILStorageTy)
OS << " (stored as " << getElementTypeName(Typed.DXILStorageTy) << ")";
OS << "\n";Either way, please add a case to llvm/test/Analysis/DXILResource/buffer-frombinding.ll that exercises this code.
llvm/lib/Analysis/DXILResource.cpp
Outdated
| } | ||
|
|
||
| static dxil::ElementType toDXILStorageType(dxil::ElementType ET) { | ||
| // TODO: Handle unorm and snorm. |
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 longer necessary - we're handling unorm and snorm right below!
|
Please don't forget to update the PR title to reflect the new name. :) |
TypedInfoTypedInfo
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/138/builds/20989 Here is the relevant piece of the build log for the reference |
In DXIL, some 64bit types are actually represented with their 32bit counterpart. This was already being address in the codegen, however the metadata generation was lacking this information. This PR is fixing this issue. Closes: [llvm#146735](llvm#146735)
In DXIL, some 64bit types are actually represented with their 32bit counterpart. This was already being address in the codegen, however the metadata generation was lacking this information. This PR is fixing this issue. Closes: [llvm#146735](llvm#146735)
In DXIL, some 64bit types are actually represented with their 32bit counterpart. This was already being address in the codegen, however the metadata generation was lacking this information. This PR is fixing this issue.
Closes: #146735