Skip to content

[GlobalISel] Catching inconsistencies in load memory, result, and range metadata type #121247

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.

Sign up for GitHub

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

Merged
merged 21 commits into from
Jan 28, 2025
Merged

[GlobalISel] Catching inconsistencies in load memory, result, and range metadata type #121247

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
d18e4f7
Catching inconsistencies in load memory, result, and range metadata type
parsifal-47 Dec 28, 2024
b76c3df
Moving test case to proper location
parsifal-47 Jan 5, 2025
b740e22
Addressing review feedback
parsifal-47 Jan 6, 2025
f935fc6
Formatting
parsifal-47 Jan 6, 2025
21d6e71
Update llvm/test/MachineVerifier/test_g_load_vector_to_scalar.mir
parsifal-47 Jan 9, 2025
ab0d2d5
Update llvm/lib/CodeGen/MachineVerifier.cpp
parsifal-47 Jan 9, 2025
21b9ca8
Addressing review feedback
parsifal-47 Jan 10, 2025
3a3233f
Formatting
parsifal-47 Jan 10, 2025
8026866
Update llvm/test/MachineVerifier/test_g_incompatible_range.mir
parsifal-47 Jan 15, 2025
80f6486
Update llvm/test/MachineVerifier/test_g_incompatible_range.mir
parsifal-47 Jan 15, 2025
314a0eb
Update llvm/lib/CodeGen/MachineVerifier.cpp
parsifal-47 Jan 15, 2025
da70608
Update llvm/test/MachineVerifier/test_g_incompatible_range.mir
parsifal-47 Jan 15, 2025
4f16afa
Update llvm/test/MachineVerifier/test_g_incompatible_range.mir
parsifal-47 Jan 15, 2025
f2ddff5
Formatting
parsifal-47 Jan 15, 2025
cd6958b
Addressing review feedback
parsifal-47 Jan 17, 2025
c4f11af
Update llvm/lib/CodeGen/MachineVerifier.cpp
parsifal-47 Jan 22, 2025
141e4b7
Reverting changes to the testcases
parsifal-47 Jan 22, 2025
b7601c0
Fix failing test
parsifal-47 Jan 23, 2025
d45d85c
Update error message in the tests
parsifal-47 Jan 24, 2025
9094137
Fixing the build
parsifal-47 Jan 24, 2025
6e0f428
Review comments
parsifal-47 Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions llvm/lib/CodeGen/MachineVerifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1274,6 +1274,16 @@ 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()) {
ConstantInt *i =
mdconst::extract<ConstantInt>(MMO.getRanges()->getOperand(0));
if (i->getIntegerType()->getBitWidth() !=
ValTy.getScalarType().getSizeInBits()) {
Copy link
Contributor

@arsenm arsenm Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link
Contributor

@arsenm arsenm Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change to use the memory type got lost

report("range is incompatible with the value it gets assigned to",
MI);
}
}
} else if (MI->getOpcode() == TargetOpcode::G_STORE) {
if (TypeSize::isKnownLT(ValTy.getSizeInBytes(),
MMO.getSize().getValue()))
Expand Down
3 changes: 2 additions & 1 deletion llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-load-memory-metadata.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
!0 = !{i24 0, i24 1048575}
!1 = !{!"omnipotent char", !2}
!2 = !{!"Simple C/C++ TBAA"}
!3 = !{i32 0, i32 1048575}
Copy link
Contributor

@arsenm arsenm Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@parsifal-47 parsifal-47 Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

@arsenm arsenm Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@parsifal-47 parsifal-47 Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, thank you!

...

# Make sure range metadata is not preserved when widening loads, but
Expand Down Expand Up @@ -67,7 +68,7 @@ body: |
; SI-NEXT: [[LOAD:%[0-9]+]]:_(s32) = G_LOAD [[COPY]](p1) :: (load (s32), !tbaa !1, addrspace 1)
; SI-NEXT: $vgpr0 = COPY [[LOAD]](s32)
%0:_(p1) = COPY $vgpr0_vgpr1
%1:_(s32) = G_LOAD %0 :: (load (s24), align 4, addrspace 1, !range !0, !tbaa !1)
%1:_(s32) = G_LOAD %0 :: (load (s24), align 4, addrspace 1, !range !3, !tbaa !1)
$vgpr0 = COPY %1

...
Expand Down
5 changes: 3 additions & 2 deletions llvm/test/CodeGen/AMDGPU/GlobalISel/regbankselect-split-scalar-load-metadata.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
!0 = !{i96 0, i96 9223372036854775808}
!1 = !{!"omnipotent char", !2}
!2 = !{!"Simple C/C++ TBAA"}
!3 = !{i32 0, i32 4294967295}
Copy link
Contributor

@arsenm arsenm Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, shouldn't need to modify the input IR

Copy link
Contributor Author

@parsifal-47 parsifal-47 Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

...

# Make sure range metadata is not preserved when widening loads, but
Expand Down Expand Up @@ -44,10 +45,10 @@ body: |
; GFX12: liveins: $sgpr0_sgpr1
; GFX12-NEXT: {{ $}}
; GFX12-NEXT: [[COPY:%[0-9]+]]:sgpr(p4) = COPY $sgpr0_sgpr1
; GFX12-NEXT: [[LOAD:%[0-9]+]]:sgpr(<3 x s32>) = G_LOAD [[COPY]](p4) :: (load (<3 x s32>), align 8, !range !0, addrspace 4)
; GFX12-NEXT: [[LOAD:%[0-9]+]]:sgpr(<3 x s32>) = G_LOAD [[COPY]](p4) :: (load (<3 x s32>), align 8
; GFX12-NEXT: $sgpr0_sgpr1_sgpr2 = COPY [[LOAD]](<3 x s32>)
%0:_(p4) = COPY $sgpr0_sgpr1
%1:_(<3 x s32>) = G_LOAD %0 :: (load (<3 x s32>), align 8, addrspace 4, !range !0)
%1:_(<3 x s32>) = G_LOAD %0 :: (load (<3 x s32>), align 8, addrspace 4, !range !3)
$sgpr0_sgpr1_sgpr2 = COPY %1

...
Expand Down
34 changes: 34 additions & 0 deletions llvm/test/MachineVerifier/test_g_incompatible_range.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# RUN: not --crash llc -mtriple=amdgcn-mesa-mesa3d -mcpu=tahiti -run-pass=none %s -filetype=null 2>&1 | FileCheck %s
--- |
define void @range_metadata_sext_i33_signed_range_i64_load_as_v2i32() {
ret void
}

!0 = !{i64 -4294967295, i64 4294967296}
!1 = !{i64 -8589934591, i64 8589934592}
Copy link
Contributor

@arsenm arsenm Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need 2 i64 ranges with different values. Just need one, or test multiple range types for different instruction types

Copy link
Contributor Author

@parsifal-47 parsifal-47 Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

range removed, thank you!


...
---
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: range is incompatible with the value it gets assigned to
%3:_(<2 x s32>) = G_LOAD %0(p1) :: (volatile load (s64), align 4, !range !1, addrspace 1)
; CHECK: Bad machine code: range is incompatible with the value it gets assigned to
%4:_(<2 x s32>) = G_LOAD %0(p1) :: (volatile load (s64), align 4, !range !0, addrspace 1)
; CHECK: Bad machine code: range is incompatible with the value it gets assigned to
%5:_(<2 x s32>) = G_LOAD %0(p1) :: (volatile load (s64), align 4, !range !0, addrspace 1)
$vgpr0_vgpr1 = COPY %3
SI_RETURN implicit $vgpr0_vgpr1
...
Loading