[PATCH] D38501: [ValueTracking] Fix a misuse of APInt in GetPointerBaseWithConstantOffset

[Michael Ferguson via Phabricator via llvm-commits]

mppf created this revision.

GetPointerBaseWithConstantOffset include this code, where ByteOffset
and GEPOffset are both of type llvm::APInt :

  ByteOffset += GEPOffset.getSExtValue();

The problem with this line is that getSExtValue() returns an int64_t, but
the += matches an overload for uint64_t. The problem is that the resulting
APInt is no longer considered to be signed. That in turn causes assertion
failures later on if the relevant pointer type is > 64 bits in width and
the GEPOffset was negative.

Changing it to

  ByteOffset += GEPOffset.sextOrTrunc(ByteOffset.getBitWidth());

resolves the issue and explicitly performs the sign-extending
or truncation.

See also
 https://reviews.llvm.org/D24729
 https://reviews.llvm.org/D24772


https://reviews.llvm.org/D38501

Files:
  lib/Analysis/ValueTracking.cpp
  test/Analysis/ValueTracking/gep-negative-issue.ll


Index: test/Analysis/ValueTracking/gep-negative-issue.ll
===================================================================
--- /dev/null
+++ test/Analysis/ValueTracking/gep-negative-issue.ll
@@ -0,0 +1,29 @@
+; RUN: opt -gvn -S < %s | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128-p100:128:64:64-p101:128:64:64"
+target triple = "x86_64-unknown-linux-gnu"
+
+%_array = type { i64, %ArrayImpl addrspace(100)*, i8 }
+
+%ArrayImpl = type { i64, i64 addrspace(100)*, [1 x i64], [1 x i64], [1 x i64], i64, i64, double addrspace(100)*, double addrspace(100)*, i8, i64 }
+
+define void @test(i64 %n_chpl) {
+entry:
+  %0 = getelementptr inbounds %_array, %_array* null, i32 0, i32 1
+  %1 = load %ArrayImpl addrspace(100)*, %ArrayImpl addrspace(100)** %0
+  %2 = getelementptr inbounds %ArrayImpl, %ArrayImpl addrspace(100)* %1, i32 0, i32 8
+  %3 = load double addrspace(100)*, double addrspace(100)* addrspace(100)* %2
+  %4 = getelementptr inbounds double, double addrspace(100)* %3, i64 -1
+  store double 0.000000e+00, double addrspace(100)* %4
+  %5 = load %ArrayImpl addrspace(100)*, %ArrayImpl addrspace(100)** %0
+  %6 = getelementptr inbounds %ArrayImpl, %ArrayImpl addrspace(100)* %5, i32 0, i32 8
+  %7 = load double addrspace(100)*, double addrspace(100)* addrspace(100)* %6
+  %8 = getelementptr inbounds double, double addrspace(100)* %7, i64 %n_chpl
+  store double 0.000000e+00, double addrspace(100)* %8
+  ret void
+; This test checks against an assertion in opt so is light on
+; checking the generated code.
+;
+; CHECK-LABEL: @test(
+; CHECK: ret void
+}
Index: lib/Analysis/ValueTracking.cpp
===================================================================
--- lib/Analysis/ValueTracking.cpp
+++ lib/Analysis/ValueTracking.cpp
@@ -3064,7 +3064,7 @@
       if (!GEP->accumulateConstantOffset(DL, GEPOffset))
         break;
 
-      ByteOffset += GEPOffset.getSExtValue();
+      ByteOffset += GEPOffset.sextOrTrunc(ByteOffset.getBitWidth());
 
       Ptr = GEP->getPointerOperand();
     } else if (Operator::getOpcode(Ptr) == Instruction::BitCast ||


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D38501.117516.patch
Type: text/x-patch
Size: 2101 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171003/7b990cea/attachment.bin>