DXIL: Use correct type ID when writing ValueAsMetadata. #94337
Conversation
|
@llvm/pr-subscribers-backend-directx Author: Tim Besard (maleadt) ChangesSmall drive-by PR (I'm not working on DXIL, but am working on a similar IR downgrader based on the opaque pointer handling developed for DXIL): It looks like the target datalayout = "e-m:e-p:32:32-i1:32-i8:8-i16:16-i32:32-i64:64-f16:16-f32:32-f64:64-n8:16:32:64"
target triple = "dxil-unknown-shadermodel6.7-library"
define void @<!-- -->kernel(ptr addrspace(1)) {
ret void
}
!kernel = !{!0}
!0 = !{ptr @<!-- -->kernel}Assembling this with IIUC, this is because the value emitted here is of type cc @bogner @llvm-beanz Full diff: https://github.com/llvm/llvm-project/pull/94337.diff 1 Files Affected:
diff --git a/llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.cpp b/llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.cpp
index bc7637a3e3def..054bcd5eaf42e 100644
--- a/llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.cpp
+++ b/llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.cpp
@@ -1348,7 +1348,7 @@ void DXILBitcodeWriter::writeValueAsMetadata(
Ty = TypedPointerType::get(F->getFunctionType(), F->getAddressSpace());
else if (GlobalVariable *GV = dyn_cast<GlobalVariable>(V))
Ty = TypedPointerType::get(GV->getValueType(), GV->getAddressSpace());
- Record.push_back(getTypeID(Ty));
+ Record.push_back(getTypeID(Ty, V));
Record.push_back(VE.getValueID(V));
Stream.EmitRecord(bitc::METADATA_VALUE, Record, 0);
Record.clear();
|
|
A similar case for loading pointer values: define void @kernel(ptr %0) {
%box = alloca ptr addrspace(1)
%x = load ptr addrspace(1), ptr %box, align 8
ret void
}This currently throws a diff --git a/llvm/lib/Bitcode/Writer50/BitcodeWriter50.cpp b/llvm/lib/Bitcode/Writer50/BitcodeWriter50.cpp
index c5c52932512f..20b58f7d1846 100644
--- a/llvm/lib/Bitcode/Writer50/BitcodeWriter50.cpp
+++ b/llvm/lib/Bitcode/Writer50/BitcodeWriter50.cpp
@@ -2845,7 +2845,7 @@ void ModuleBitcodeWriter50::writeInstruction(const Instruction &I,
if (!pushValueAndType(I.getOperand(0), InstID, Vals)) // ptr
AbbrevToUse = FUNCTION_INST_LOAD_ABBREV;
}
- Vals.push_back(getTypeID(I.getType()));
+ Vals.push_back(getTypeID(I.getType(), &I));
Vals.push_back(getEncodedAlign(cast<LoadInst>(I).getAlign()));
Vals.push_back(cast<LoadInst>(I).isVolatile());
if (cast<LoadInst>(I).isAtomic()) {And another for define i64 @kernel2(ptr %0, ptr %1, i1 %cond) {
entry:
br i1 %cond, label %trueBlock, label %falseBlock
trueBlock:
br label %select
falseBlock:
br label %select
select:
%ifelse = phi ptr [ %0, %trueBlock ], [ %1, %falseBlock ]
%value = load i64, ptr %ifelse
ret i64 %value
}Resulting in an I guess the DXIL serializer isn't really made for this use case, presumably because it doesn't support pointer values (as I understand from a comment in the code)? |
There was a problem hiding this comment.
Pointer values are a bit interesting because DXIL (and its usual source language, HLSL) don't generally expose pointers, so these code paths are probably not really reachable without fiddling with the IR we input to the DXIL backend. That said, I think it's reasonable to take the obvious fixes either way.
It would be better if this came with a test. Can you come up with something sensible or do you need someone (like me) with access to dxil-dis to take a stab at that?
Yeah, I noticed, and there turned out to be several more issues than the ones mentioned here. I ultimately took another approach, instead of inferring pointer types based on some later use (which can lead to conflicts that fall back to I'll see about adding a test for the changes in here. |
|
Unfortunately the always casting to and from the typed pointer doesn't work for DXIL because DXIL disallows bitcasts for a lot of cases, and that is enforced by the DXIL validator which operates on typed pointer IR. |
|
Sorry for the delay. Added a test (verifying that the test crashes without this change), and also changed the |
|
@bogner Anything else you want me to add here? |
|
Feel free to merge, I don't have commit bit. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/1127 Here is the relevant piece of the build log for the reference |
Small drive-by PR (I'm not working on DXIL, but am working on a similar IR downgrader based on the opaque pointer handling developed for DXIL): It looks like the
ValueAsMetadatahandling currently is broken when writing references to functions. For example:Assembling this with
llcand disassembling the contained IR withllvm-disfrom LLVM 12 (again, I'm not working on DXIL so I don't havedxil-dishandy) breaks:IIUC, this is because the value emitted here is of type
typedptr(void (ptr addrspace(1)), 0), while the type is simplyvoid (i8*)*because it's not looked up in the PointerAnalysisMap. PassingValong fixes this.cc @bogner @llvm-beanz