[AsmWriter] Fix MIR printing of single constant LLVM IR metadata #165029
Conversation
When LLVM IR is read in and its metadata is canonicalized into Value objects, single constant metadata are not turned into MDNode objects like every other metadata. Instead, they are left as ConstantAsMetadata objects. When the ModuleSlotTracker creates slots for MDNode objects, it thus does not make one for the constant. However, during translation into MIR that constant metadata will be turned into an MDNode because that is the only Metadata object that MachineInstruction objects can house. So when MIRPrintingPass makes use of that ModuleSlotTracker, it will pass it an MDNode corresponding to that constant that never got a slot. This causes the pass to dump a pointer instead. This change fixes this issue by updating the writeAsOperandInternal function to catch the case where it cannot find the slot for the solo constant metadata node and print the constant itself directly instead of dumping the pointer. Signed-off-by: Domenic Nutile <[email protected]>
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-llvm-ir Author: None (saxlungs) ChangesWhen LLVM IR is read in and its metadata are canonicalized into Value objects, single constant metadata are not turned into MDNode objects like every other metadata. Instead, they are left as ConstantAsMetadata objects. When the ModuleSlotTracker creates slots for MDNode objects, it thus does not make one for the constant. However, during translation into MIR that constant metadata will be turned into an MDNode because that is the only Metadata object that MachineInstruction objects can house. So when MIRPrintingPass makes use of that ModuleSlotTracker, it will pass it an MDNode corresponding to that constant that never got a slot. This causes the pass to dump a pointer instead. This change fixes this issue by updating the writeAsOperandInternal function to catch the case where it cannot find the slot for the solo constant metadata node and print the constant itself directly instead of dumping the pointer. Full diff: https://github.com/llvm/llvm-project/pull/165029.diff 2 Files Affected:
diff --git a/llvm/lib/IR/AsmWriter.cpp b/llvm/lib/IR/AsmWriter.cpp
index 1096e57632d97..20264f473ad56 100644
--- a/llvm/lib/IR/AsmWriter.cpp
+++ b/llvm/lib/IR/AsmWriter.cpp
@@ -2810,6 +2810,12 @@ static void writeAsOperandInternal(raw_ostream &Out, const Metadata *MD,
writeDILocation(Out, Loc, WriterCtx);
return;
}
+ if (N->getNumOperands() == 1) {
+ if (const auto *CAM = dyn_cast<ConstantAsMetadata>(N->getOperand(0))) {
+ writeConstantInternal(Out, CAM->getValue(), WriterCtx);
+ return;
+ }
+ }
// Give the pointer value instead of "badref", since this comes up all
// the time when debugging.
Out << "<" << N << ">";
diff --git a/llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-metadata.ll b/llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-metadata.ll
index 101bb6c0ed123..149395830450d 100644
--- a/llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-metadata.ll
+++ b/llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-metadata.ll
@@ -6,8 +6,7 @@ define i32 @reloc_constant() {
; CHECK-LABEL: name: reloc_constant
; CHECK: bb.1 (%ir-block.0):
; CHECK-NEXT: [[INT:%[0-9]+]]:_(s32) = G_INTRINSIC intrinsic(@llvm.amdgcn.reloc.constant), !0
- ; We cannot have any specific metadata check here as ConstantAsMetadata is printed as <raw_ptr_val>
- ; CHECK-NEXT: [[INT1:%[0-9]+]]:_(s32) = G_INTRINSIC intrinsic(@llvm.amdgcn.reloc.constant), <0x{{[0-9a-f]+}}>
+ ; CHECK-NEXT: [[INT1:%[0-9]+]]:_(s32) = G_INTRINSIC intrinsic(@llvm.amdgcn.reloc.constant), 4
; CHECK-NEXT: [[ADD:%[0-9]+]]:_(s32) = G_ADD [[INT]], [[INT1]]
; CHECK-NEXT: $vgpr0 = COPY [[ADD]](s32)
; CHECK-NEXT: SI_RETURN implicit $vgpr0
|
|
@llvm/pr-subscribers-llvm-globalisel Author: None (saxlungs) ChangesWhen LLVM IR is read in and its metadata are canonicalized into Value objects, single constant metadata are not turned into MDNode objects like every other metadata. Instead, they are left as ConstantAsMetadata objects. When the ModuleSlotTracker creates slots for MDNode objects, it thus does not make one for the constant. However, during translation into MIR that constant metadata will be turned into an MDNode because that is the only Metadata object that MachineInstruction objects can house. So when MIRPrintingPass makes use of that ModuleSlotTracker, it will pass it an MDNode corresponding to that constant that never got a slot. This causes the pass to dump a pointer instead. This change fixes this issue by updating the writeAsOperandInternal function to catch the case where it cannot find the slot for the solo constant metadata node and print the constant itself directly instead of dumping the pointer. Full diff: https://github.com/llvm/llvm-project/pull/165029.diff 2 Files Affected:
diff --git a/llvm/lib/IR/AsmWriter.cpp b/llvm/lib/IR/AsmWriter.cpp
index 1096e57632d97..20264f473ad56 100644
--- a/llvm/lib/IR/AsmWriter.cpp
+++ b/llvm/lib/IR/AsmWriter.cpp
@@ -2810,6 +2810,12 @@ static void writeAsOperandInternal(raw_ostream &Out, const Metadata *MD,
writeDILocation(Out, Loc, WriterCtx);
return;
}
+ if (N->getNumOperands() == 1) {
+ if (const auto *CAM = dyn_cast<ConstantAsMetadata>(N->getOperand(0))) {
+ writeConstantInternal(Out, CAM->getValue(), WriterCtx);
+ return;
+ }
+ }
// Give the pointer value instead of "badref", since this comes up all
// the time when debugging.
Out << "<" << N << ">";
diff --git a/llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-metadata.ll b/llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-metadata.ll
index 101bb6c0ed123..149395830450d 100644
--- a/llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-metadata.ll
+++ b/llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-metadata.ll
@@ -6,8 +6,7 @@ define i32 @reloc_constant() {
; CHECK-LABEL: name: reloc_constant
; CHECK: bb.1 (%ir-block.0):
; CHECK-NEXT: [[INT:%[0-9]+]]:_(s32) = G_INTRINSIC intrinsic(@llvm.amdgcn.reloc.constant), !0
- ; We cannot have any specific metadata check here as ConstantAsMetadata is printed as <raw_ptr_val>
- ; CHECK-NEXT: [[INT1:%[0-9]+]]:_(s32) = G_INTRINSIC intrinsic(@llvm.amdgcn.reloc.constant), <0x{{[0-9a-f]+}}>
+ ; CHECK-NEXT: [[INT1:%[0-9]+]]:_(s32) = G_INTRINSIC intrinsic(@llvm.amdgcn.reloc.constant), 4
; CHECK-NEXT: [[ADD:%[0-9]+]]:_(s32) = G_ADD [[INT]], [[INT1]]
; CHECK-NEXT: $vgpr0 = COPY [[ADD]](s32)
; CHECK-NEXT: SI_RETURN implicit $vgpr0
|
arsenm
left a comment
arsenm
left a comment
There was a problem hiding this comment.
This seems like too low level of a change. Why are these cases not just uniformly handled in the first place?
It seems to me that "MDNode" was originally specifically meant to be just used for metadata in the LLVM IR that's in the format "!x", where x is some number. This makes sense for SlotTracker purposes, because those metadata also later have the definition of what each index is referring to (you can see an example in the test I updated). However, MachineInstruction objects don't just house generic Metadata objects, for some reason their "Contents" field enum only allows MDNode objects specifically. So any metadata that isn't an MDNode that wants to be kept in the MIR just gets wrapped in an MDNode before getting put into a MachineInstruction during translation (you can see this around line ~2845 in llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp). To truly solve the issue, I think we'd have to allow any Metadata family object to be in the MachineInstruction, but I don't know why that limitation was made in the first place and I can imagine that that might be a very involved change if they had good reasons. Since this is just meant to fix some small headaches when updating tests I thought the minimal effective solution could be good, but let me know if you think a much deeper dive would be appropriate |
|
Ping |
I don't know why this boxing happens, but I'd expect this unboxing to happen in some MIR printer context, not in the underlying IR printer |
3 participants
When LLVM IR is read in and its metadata are canonicalized into Value objects, single constant metadata are not turned into MDNode objects like every other metadata. Instead, they are left as ConstantAsMetadata objects. When the ModuleSlotTracker creates slots for MDNode objects, it thus does not make one for the constant. However, during translation into MIR that constant metadata will be turned into an MDNode because that is the only Metadata object that MachineInstruction objects can house. So when MIRPrintingPass makes use of that ModuleSlotTracker, it will pass it an MDNode corresponding to that constant that never got a slot. This causes the pass to dump a pointer instead.
This change fixes this issue by updating the writeAsOperandInternal function to catch the case where it cannot find the slot for the solo constant metadata node and print the constant itself directly instead of dumping the pointer.