[GlobalISel] Catching inconsistencies in load memory, result, and range metadata type #121247
Conversation
|
@llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-llvm-globalisel Author: Renat Idrisov (parsifal-47) ChangesThis is a fix for: Full diff: https://github.com/llvm/llvm-project/pull/121247.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index bec36b728ae328..1866d3ae2b285a 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -1274,6 +1274,12 @@ void MachineVerifier::verifyPreISelGenericInstruction(const MachineInstr *MI) {
if (TypeSize::isKnownGT(MMO.getSize().getValue(),
ValTy.getSizeInBytes()))
report("load memory size cannot exceed result size", MI);
+
+ if (MMO.getRanges() && (ValTy.isVector() != MMO.getType().isVector())) {
+ report("scalar operands cannot be loaded into vector values and vice "
+ "versa",
+ MI);
+ }
} else if (MI->getOpcode() == TargetOpcode::G_STORE) {
if (TypeSize::isKnownLT(ValTy.getSizeInBytes(),
MMO.getSize().getValue()))
diff --git a/llvm/test/CodeGen/AMDGPU/GlobalISel/test_g_load_vector_to_scalar.mir b/llvm/test/CodeGen/AMDGPU/GlobalISel/test_g_load_vector_to_scalar.mir
new file mode 100644
index 00000000000000..95429127c30d0c
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/GlobalISel/test_g_load_vector_to_scalar.mir
@@ -0,0 +1,25 @@
+# RUN: not --crash llc -mtriple=amdgcn-mesa-mesa3d -mcpu=tahiti -run-pass=amdgpu-prelegalizer-combiner -verify-machineinstrs %s -o -
+--- |
+ define void @range_metadata_sext_i33_signed_range_i64_load_as_v2i32() {
+ ret void
+ }
+
+ !1 = !{i64 -8589934591, i64 8589934592}
+
+...
+---
+name: range_metadata_sext_i33_signed_range_i64_load_as_v2i32
+tracksRegLiveness: true
+body: |
+ bb.0:
+ liveins: $vgpr0, $vgpr1
+
+ %1:_(s32) = COPY $vgpr0
+ %2:_(s32) = COPY $vgpr1
+ %0:_(p1) = G_MERGE_VALUES %1(s32), %2(s32)
+ ; CHECK: Bad machine code: scalar operands cannot be loaded into vector values
+ %3:_(<2 x s32>) = G_LOAD %0(p1) :: (volatile load (s64), align 4, !range !1, addrspace 1)
+ $vgpr0_vgpr1 = COPY %3
+ SI_RETURN implicit $vgpr0_vgpr1
+
+...
|
|
@arsenm please take a look when you have a chance, thank you! |
|
|
|
The failure does not look related to the change: @arsenm please take a look when you have a chance, thank you! |
| @@ -0,0 +1,25 @@ | |||
| # RUN: not --crash llc -mtriple=amdgcn-mesa-mesa3d -mcpu=tahiti -run-pass=amdgpu-prelegalizer-combiner -verify-machineinstrs %s -o - | |||
There was a problem hiding this comment.
This belongs in test/MachineVerifier, and can use -run-pass=none (and does not need -verify-machineinstrs).
You should also include all the cases in the test I added to the bug (you'll probably need to merge the different MIR instructions into one function to test them in one mir file)
There was a problem hiding this comment.
Moved and updated, thank you!
llvm/lib/CodeGen/MachineVerifier.cpp
Outdated
| ValTy.getSizeInBytes())) | ||
| report("load memory size cannot exceed result size", MI); | ||
|
|
||
| if (MMO.getRanges() && (ValTy.isVector() != MMO.getType().isVector())) { |
There was a problem hiding this comment.
This isn't quite right. You're checking a different condition than #97290 is asking for. Neither of the checked types are from MMO.getRanges. We probably should have a similar check, without requiring ranges, but I believe we're relying on the current behavior so it's beyond the scope of this patch.
Instead you should be verifying the IR type in MMO.getRanges() against ValTy and MMO.getType()
There was a problem hiding this comment.
This is something I likely do not understand,
I can get a type from range for example like this:
mdconst::dyn_extract<ConstantInt>(MMO.getRanges()->getOperand(0))->getType()
but there is no other option but ConstantInt, so it cannot be vector, and that was the reason I checked for range presence because I was thinking it indicates scalar nature of the value.
Let's assume, I have this type now, what should I check for, should int type of the range be equal to ValTy?
Should I check all three to be the same? Or should I just check for vector vs scalar mismatches?
Thank you for your help!
There was a problem hiding this comment.
You get the scalar integer type out of mdconst::extract<ConstantInt>(MMO.getRanges()->getOperand(0))->getType(). You need to verify the MIR result type has the same scalar type, something like ValTy.getScalarType()->getSizeInBits() == mdconst::extract<ConstantInt>(MMO.getRanges()->getOperand(0)).getIntegerType()->getBitWidth(). Whether the MIR type is vector isn't as significant as whether the element type matches the range IR type
There was a problem hiding this comment.
I am not sure I understood it correctly, if I get the bitWidth, it will be 64 in all three cases and types will be compatible, but if I take actual bits that are used by lower and higher bounds of the range, it breaks existing tests. Please take a look at the updated code and let me know what am I missing. Thank you for your patience!
There was a problem hiding this comment.
This set of IR tests happens to only use 64-bit values
, it breaks existing tests.
It's possible there are broken tests or bugs, that's why we write these verifiers
There was a problem hiding this comment.
I have made fixes to two newly failing tests, please take a look if I understand the way to do it, thank you!
parsifal-47
force-pushed
the
machine-verifier-load-inconsistencies
branch
from
565ba0e to
b76c3df
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| %1:_(s32) = COPY $vgpr0 | ||
| %2:_(s32) = COPY $vgpr1 | ||
| %0:_(p1) = G_MERGE_VALUES %1(s32), %2(s32) | ||
| ; CHECK: Bad machine code: range is incompatible with the value it gets assigned to |
There was a problem hiding this comment.
I think the verifier aborts after the first function fails, but keeps printing errors in the functions. As a consequence you would need all of these cases tested in one function
There was a problem hiding this comment.
not sure if I understood it correctly, I put them all in one function, but now they look too similar, please take a look, thank you!
llvm/lib/CodeGen/MachineVerifier.cpp
Outdated
| ConstantInt *i = mdconst::dyn_extract<ConstantInt>(o); | ||
| if (!i->isNegative()) | ||
| return i->getValue().getActiveBits(); | ||
| APInt reversed(i->getValue()); |
There was a problem hiding this comment.
You shouldn't need to consider anything about the value of the range, only the type
There was a problem hiding this comment.
This is something I still do not understand:
The verifier should reject cases like this:
...
!0 = !{i64 -4294967295, i64 4294967296}
...
%3:_(<2 x s32>) = G_LOAD %0(p1) :: (volatile load (s64), align 4, !range !0, addrspace 1)
but if we get the information about the type, it is "i64" and there is a match between 2*32 == 64, no issues, you mentioned vector or scalar is not important, what would be the reason to reject this instruction?
There was a problem hiding this comment.
@arsenm could you please help me understand this piece, it looks like if I rely on type information alone, the cases that are mentioned in the description of this bug would be still legal. Thank you!
There was a problem hiding this comment.
there is a match between 2*32 == 64
This is the problem, it's not a match. The total bitwidth is not what matters, the scalar type does. It would be OK if this was a <2 x s64> result and the range was using i64. tttt
i.e. the vectorness doesn't matter, nor the total bit width. The scalar type interpretation on both halves is what matters
There was a problem hiding this comment.
thank you for the explanation, I will get back with updated code
There was a problem hiding this comment.
updated, please take a look
Co-authored-by: Matt Arsenault <[email protected]>
Co-authored-by: Matt Arsenault <[email protected]>
|
@arsenm please take a look when you have a chance, I think I resolved all your comments, but feel free to add more, thank you! |
arsenm
left a comment
arsenm
left a comment
There was a problem hiding this comment.
Change essentially LGTM with some rephrasing of the message and some more variety in the test
Co-authored-by: Matt Arsenault <[email protected]>
Co-authored-by: Matt Arsenault <[email protected]>
Co-authored-by: Matt Arsenault <[email protected]>
Co-authored-by: Matt Arsenault <[email protected]>
Co-authored-by: Matt Arsenault <[email protected]>
Thank you for the review and code suggestions, all should be resolved now! |
arsenm
left a comment
arsenm
left a comment
There was a problem hiding this comment.
Close, just a little more test cleanup
| !0 = !{i24 0, i24 1048575} | ||
| !1 = !{!"omnipotent char", !2} | ||
| !2 = !{!"Simple C/C++ TBAA"} | ||
| !3 = !{i32 0, i32 1048575} |
There was a problem hiding this comment.
This shouldn't touch the input IR. However this makes me realize we are violating the principle that we shouldn't touch the underlying IR in codegen. I guess global metadata constants could be an exception
There was a problem hiding this comment.
sorry, I do not understand how to resolve the comment, this test started to fail because i24 range is used on i32 value, I have forked this range with i32 to make the test pass, should I change this test or should I change the check? Thank you!
There was a problem hiding this comment.
I guess it depends how we want to handle extending loads. As written, I think we should leave the test for now. However that means you need to adjust from checking the result memory type, to the memory type in the MachineMemOperand
There was a problem hiding this comment.
got it, thank you!
| !0 = !{i96 0, i96 9223372036854775808} | ||
| !1 = !{!"omnipotent char", !2} | ||
| !2 = !{!"Simple C/C++ TBAA"} | ||
| !3 = !{i32 0, i32 4294967295} |
There was a problem hiding this comment.
Same, shouldn't need to modify the input IR
There was a problem hiding this comment.
the instruction here:
%1:_(<3 x s32>) = G_LOAD %0 :: (load (<3 x s32>), align 8, addrspace 4, !range !3)
how would you recommend to alter the test to make it pass?
| } | ||
|
|
||
| !0 = !{i64 -4294967295, i64 4294967296} | ||
| !1 = !{i64 -8589934591, i64 8589934592} |
There was a problem hiding this comment.
We don't need 2 i64 ranges with different values. Just need one, or test multiple range types for different instruction types
There was a problem hiding this comment.
range removed, thank you!
| !0 = !{i24 0, i24 1048575} | ||
| !1 = !{!"omnipotent char", !2} | ||
| !2 = !{!"Simple C/C++ TBAA"} | ||
| !3 = !{i32 0, i32 1048575} |
There was a problem hiding this comment.
I guess it depends how we want to handle extending loads. As written, I think we should leave the test for now. However that means you need to adjust from checking the result memory type, to the memory type in the MachineMemOperand
| ConstantInt *i = | ||
| mdconst::extract<ConstantInt>(MMO.getRanges()->getOperand(0)); | ||
| if (i->getIntegerType()->getBitWidth() != | ||
| ValTy.getScalarType().getSizeInBits()) { |
There was a problem hiding this comment.
| ValTy.getScalarType().getSizeInBits()) { | |
| MMO.getMemoryType().getScalarSizeInBits()) { |
This should continue to permit the extending load case to use range metadata. Verifying the result type is trickier since we need to account for valid extending loads
There was a problem hiding this comment.
The change to use the memory type got lost
Co-authored-by: Matt Arsenault <[email protected]>
|
I noticed that tests are now failing, let me check the details and get back to you |
|
We probably should do something about verifying the result types also are consistent, but that will probably require touching the legalizer. Plus we should verify that computeKnownBits does the right thing in the extended result cases |
|
Test still failing |
Should be good now, but
Let me know what do you think, thank you! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/16803 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/13645 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/12107 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/12110 Here is the relevant piece of the build log for the reference |
|
thank you @arsenm ! |
Great, thank you for your help and support! |
|
@arsenm , @parsifal-47 , looks like these changes cause a failure of `LLVM :: CodeGen/AArch64/setcc_knownbits.ll' test on the expensive checks builder here I found a problematic commit by bisecting though the commits for this buildbot's build. Reverting of these changes fixes the test. |
|
Fix in #124894 |
5 participants
This is a fix for:
#97290
Please let me know if that is the right way to address the issue. Thank you!