[PATCH] D99481: [InstCombine] Fix miscompile on GEP+load to icmp fold (PR45210)

[Hyeongyu Kim via Phabricator via llvm-commits]

hyeongyukim updated this revision to Diff 351655.
hyeongyukim added a comment.

Add a mask to Idx in case of magic_cst.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99481/new/

https://reviews.llvm.org/D99481

Files:
  llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
  llvm/test/Transforms/InstCombine/load-cmp.ll


Index: llvm/test/Transforms/InstCombine/load-cmp.ll
===================================================================
--- llvm/test/Transforms/InstCombine/load-cmp.ll
+++ llvm/test/Transforms/InstCombine/load-cmp.ll
@@ -299,7 +299,6 @@
 }
 
 define i1 @test10_struct_arr_noinbounds_i16(i16 %x) {
-; CHECK-LABEL: @test10_struct_arr_noinbounds_i16(
 ; CHECK-NEXT:    [[TMP1:%.*]] = sext i16 [[X:%.*]] to i32
 ; CHECK-NEXT:    [[TMP2:%.*]] = and i32 [[TMP1]], 268435455
 ; CHECK-NEXT:    [[R:%.*]] = icmp ne i32 [[TMP2]], 1
Index: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -277,27 +277,30 @@
     unsigned PtrSize = IntPtrTy->getIntegerBitWidth();
     if (Idx->getType()->getPrimitiveSizeInBits().getFixedSize() > PtrSize)
       Idx = Builder.CreateTrunc(Idx, IntPtrTy);
+  }
 
-    unsigned ElementSize =
-        DL.getTypeAllocSize(Init->getType()->getArrayElementType());
-
-    // If inbounds keyword is not present, Idx * ElementSize can overflow.
-    // Let's assume that ElementSize is 2 and the wanted value is at offset 0.
-    // Then, there are two possible values for Idx to match offset 0:
-    // 0x00..00, 0x80..00.
-    // Emitting 'icmp eq Idx, 0' isn't correct in this case because the
-    // comparison is false if Idx was 0x80..00.
-    // We need to erase the highest countTrailingZeros(ElementSize) bits of Idx.
-    if (countTrailingZeros(ElementSize) != 0) {
-      Value *Mask = ConstantInt::getSigned(Idx->getType(), -1);
+  // If inbounds keyword is not present, Idx * ElementSize can overflow.
+  // Let's assume that ElementSize is 2 and the wanted value is at offset 0.
+  // Then, there are two possible values for Idx to match offset 0:
+  // 0x00..00, 0x80..00.
+  // Emitting 'icmp eq Idx, 0' isn't correct in this case because the
+  // comparison is false if Idx was 0x80..00.
+  // We need to erase the highest countTrailingZeros(ElementSize) bits of Idx.
+  unsigned ElementSize =
+      DL.getTypeAllocSize(Init->getType()->getArrayElementType());
+  auto MaskIdx = [&](Value* Idx){
+    if (!GEP->isInBounds() && countTrailingZeros(ElementSize) != 0) {
+      Value *Mask = ConstantInt::get(Idx->getType(), -1);
       Mask = Builder.CreateLShr(Mask, countTrailingZeros(ElementSize));
       Idx = Builder.CreateAnd(Idx, Mask);
     }
-  }
+    return Idx;
+  };
 
   // If the comparison is only true for one or two elements, emit direct
   // comparisons.
   if (SecondTrueElement != Overdefined) {
+    Idx = MaskIdx(Idx);
     // None true -> false.
     if (FirstTrueElement == Undefined)
       return replaceInstUsesWith(ICI, Builder.getFalse());
@@ -318,6 +321,7 @@
   // If the comparison is only false for one or two elements, emit direct
   // comparisons.
   if (SecondFalseElement != Overdefined) {
+    Idx = MaskIdx(Idx);
     // None false -> true.
     if (FirstFalseElement == Undefined)
       return replaceInstUsesWith(ICI, Builder.getTrue());
@@ -339,6 +343,7 @@
   // where it is true, emit the range check.
   if (TrueRangeEnd != Overdefined) {
     assert(TrueRangeEnd != FirstTrueElement && "Should emit single compare");
+    Idx = MaskIdx(Idx);
 
     // Generate (i-FirstTrue) <u (TrueRangeEnd-FirstTrue+1).
     if (FirstTrueElement) {
@@ -354,6 +359,7 @@
   // False range check.
   if (FalseRangeEnd != Overdefined) {
     assert(FalseRangeEnd != FirstFalseElement && "Should emit single compare");
+    Idx = MaskIdx(Idx);
     // Generate (i-FirstFalse) >u (FalseRangeEnd-FirstFalse).
     if (FirstFalseElement) {
       Value *Offs = ConstantInt::get(Idx->getType(), -FirstFalseElement);
@@ -369,6 +375,7 @@
   // of this load, replace it with computation that does:
   //   ((magic_cst >> i) & 1) != 0
   {
+    Idx = MaskIdx(Idx);
     Type *Ty = nullptr;
 
     // Look for an appropriate type:


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D99481.351655.patch
Type: text/x-patch
Size: 3983 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210612/3b28cb88/attachment.bin>