search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.26k stars 11.67k forks source link

MachineVerifier should catch inconsistencies in load memory, result, and range metadata type. #97290

Open arsenm opened 3 months ago

arsenm commented 3 months ago

The MachineVerifier does not catch cases where G_LOAD range metadata is applied to scalar memory types, with vector results and vice versa. The verifier should reject cases like this: 

# RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=tahiti -run-pass=amdgpu-prelegalizer-combiner -verify-machineinstrs %s -o -

--- |
  define void @range_metadata_sext_i8_signed_range_i64_load_as_v2i32() {
   ret void
  }

  define void @range_metadata_sext_i8_signed_range_i64_load_as_v2i32_extractlo() {
   ret void
  }

  define void @range_metadata_sext_i33_signed_range_i64_load_as_v2i32() {
    ret void
  }

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

...
---
name:            range_metadata_sext_i8_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)
    %3:_(<2 x s32>) = G_LOAD %0(p1) :: (volatile load (s64), align 4, !range !0, addrspace 1)
    %6:_(<2 x s32>) = G_SEXT_INREG %3, 9
    $vgpr0_vgpr1 = COPY %6
    SI_RETURN implicit $vgpr0_vgpr1

...

---
name:            range_metadata_sext_i8_signed_range_i64_load_as_v2i32_extractlo
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)
    %3:_(<2 x s32>) = G_LOAD %0(p1) :: (volatile load (s64), align 4, !range !0, addrspace 1)
    %zero:_(s32) = G_CONSTANT i32 0
    %extract:_(s32) = G_EXTRACT_VECTOR_ELT %3, %zero
    %6:_(s32) = G_SEXT_INREG %zero, 9
    $vgpr0 = COPY %6
    SI_RETURN implicit $vgpr0, implicit $vgpr1

...

---
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)
    %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

...

Here we have an i64 range value applied to a <2 x i32> result, which should be invalid. %3:_(<2 x s32>) = G_LOAD %0(p1) :: (volatile load (s64), align 4, !range !0, addrspace 1)

We also are not clean about ensuring we have vector element matches between the result and memory type, but I think that's a more difficult issue to fix than verifying the range metadata.

llvmbot commented 3 months ago

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

  1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
  2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
  3. Fix the issue locally.
  4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
  5. Create a Git commit.
  6. Run git clang-format HEAD~1 to format your changes.
  7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

llvmbot commented 3 months ago

@llvm/issue-subscribers-good-first-issue

Author: Matt Arsenault (arsenm)

The MachineVerifier does not catch cases where G_LOAD range metadata is applied to scalar memory types, with vector results and vice versa. The verifier should reject cases like this: ``` # RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=tahiti -run-pass=amdgpu-prelegalizer-combiner -verify-machineinstrs %s -o - --- | define void @range_metadata_sext_i8_signed_range_i64_load_as_v2i32() { ret void } define void @range_metadata_sext_i8_signed_range_i64_load_as_v2i32_extractlo() { ret void } define void @range_metadata_sext_i33_signed_range_i64_load_as_v2i32() { ret void } !0 = !{i64 -4294967295, i64 4294967296} !1 = !{i64 -8589934591, i64 8589934592} ... --- name: range_metadata_sext_i8_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) %3:_(<2 x s32>) = G_LOAD %0(p1) :: (volatile load (s64), align 4, !range !0, addrspace 1) %6:_(<2 x s32>) = G_SEXT_INREG %3, 9 $vgpr0_vgpr1 = COPY %6 SI_RETURN implicit $vgpr0_vgpr1 ... --- name: range_metadata_sext_i8_signed_range_i64_load_as_v2i32_extractlo 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) %3:_(<2 x s32>) = G_LOAD %0(p1) :: (volatile load (s64), align 4, !range !0, addrspace 1) %zero:_(s32) = G_CONSTANT i32 0 %extract:_(s32) = G_EXTRACT_VECTOR_ELT %3, %zero %6:_(s32) = G_SEXT_INREG %zero, 9 $vgpr0 = COPY %6 SI_RETURN implicit $vgpr0, implicit $vgpr1 ... --- 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) %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 ... ``` Here we have an i64 range value applied to a <2 x i32> result, which should be invalid. %3:_(<2 x s32>) = G_LOAD %0(p1) :: (volatile load (s64), align 4, !range !0, addrspace 1) We also are not clean about ensuring we have vector element matches between the result and memory type, but I think that's a more difficult issue to fix than verifying the range metadata.
JOE1994 commented 3 months ago

Can I give it a try? I have no experience with globalisel.

AditiRM commented 1 month ago

@JOE1994 Are you still working on the issue ? If not, I would love to work on the same. Please let me know.

JOE1994 commented 1 month ago

@JOE1994 Are you still working on the issue ? If not, I would love to work on the same. Please let me know.

Please feel free to work on this issue. I'm unavailable to work on this in the next 2~3 weeks..