[llvm] [SeparateConstOffsetFromGEP] Fix incorrect inbounds flag in case of non-negative index but negative offset (PR #190192)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-backend-risc-v

Author: Sergey Kachkov (skachkov-sc)

<details>
<summary>Changes</summary>

Fixes #<!-- -->190187 

Currently, SeparateConstOffsetFromGEP preserves inbounds attribute if new sequence of GEPs from the same base pointer has non-negative offsets in each GEP (this was mentioned in https://github.com/llvm/llvm-project/pull/159515). This statement seems correct for me (if the sequence consists from 2 GEPs), but current implementation has a flaw: it checks that constant byte offset and GEP indices are non-negative. However, in some corner cases we can have a situation when the index is non-negative, but its offset (in bytes) is negative, so we can't preserve inbounds attribute. In the example, GEP index after transformation can have values `0x7ffffffffffffffd`/`0x7ffffffffffffffe`/`0x7fffffffffffffff`; they are all non-negative (sign bit is zero), however, after multiplication on sizeof(i64) they become negative and inbounds can't be preserved anymore.

The proposed fix is to check that Idx * ElementStride is non-negative (instead of checking Idx only).

---
Full diff: https://github.com/llvm/llvm-project/pull/190192.diff


2 Files Affected:

- (modified) llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp (+7-2) 
- (modified) llvm/test/Transforms/SeparateConstOffsetFromGEP/RISCV/split-gep.ll (+21) 


``````````diff
diff --git a/llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp b/llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp
index 5a38f11314b13..84ad792f44a7b 100644
--- a/llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp
+++ b/llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp
@@ -1178,8 +1178,13 @@ bool SeparateConstOffsetFromGEP::splitGEP(GetElementPtrInst *GEP) {
         Idx = NewIdx;
         AllNUWPreserved &= PreservesNUW;
       }
-      AllOffsetsNonNegative =
-          AllOffsetsNonNegative && isKnownNonNegative(Idx, *DL);
+      // Ensure that index offset in bytes (Idx * SequentialElementStride) is
+      // non-negative. If Idx value has at least log2(Stride) + 1 zeros, it
+      // guarantees that (Idx * Stride) will have zeroed sign bit.
+      bool OffsetNonNegative =
+          computeKnownBits(Idx, *DL).countMinLeadingZeros() >=
+          Log2_64_Ceil(GTI.getSequentialElementStride(*DL)) + 1;
+      AllOffsetsNonNegative = AllOffsetsNonNegative && OffsetNonNegative;
     }
   }
   if (ExtractBase) {
diff --git a/llvm/test/Transforms/SeparateConstOffsetFromGEP/RISCV/split-gep.ll b/llvm/test/Transforms/SeparateConstOffsetFromGEP/RISCV/split-gep.ll
index deaffc88117dd..f696f89e2f45c 100644
--- a/llvm/test/Transforms/SeparateConstOffsetFromGEP/RISCV/split-gep.ll
+++ b/llvm/test/Transforms/SeparateConstOffsetFromGEP/RISCV/split-gep.ll
@@ -289,3 +289,24 @@ entry:
   store i32 %i, ptr %gep8
   ret i32 undef
 }
+
+define i64 @test_inbounds(ptr %arr, i64 %x) {
+; CHECK-LABEL: @test_inbounds(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[MIN:%.*]] = tail call i64 @llvm.umin.i64(i64 [[X:%.*]], i64 4)
+; CHECK-NEXT:    [[XOR:%.*]] = xor i64 [[MIN]], -1
+; CHECK-NEXT:    [[SHR:%.*]] = lshr i64 [[XOR]], 1
+; CHECK-NEXT:    [[TMP0:%.*]] = getelementptr [6 x i64], ptr [[ARR:%.*]], i64 0, i64 [[SHR]]
+; CHECK-NEXT:    [[GEP2:%.*]] = getelementptr i8, ptr [[TMP0]], i64 40
+; CHECK-NEXT:    [[RES:%.*]] = load i64, ptr [[GEP2]], align 8
+; CHECK-NEXT:    ret i64 [[RES]]
+;
+entry:
+  %min = tail call i64 @llvm.umin.i64(i64 %x, i64 4)
+  %xor = xor i64 %min, -1
+  %shr = lshr i64 %xor, 1
+  %sub = add nsw i64 %shr, -9223372036854775803
+  %gep = getelementptr inbounds nuw [6 x i64], ptr %arr, i64 0, i64 %sub
+  %res = load i64, ptr %gep, align 8
+  ret i64 %res
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/190192