[llvm] r350395 - [ValueTracking] Fix a misuse of APInt in GetPointerBaseWithConstantOffset

[Florian Hahn via llvm-commits]

Author: fhahn
Date: Fri Jan  4 06:53:22 2019
New Revision: 350395

URL: http://llvm.org/viewvc/llvm-project?rev=350395&view=rev
Log:
[ValueTracking] Fix a misuse of APInt in GetPointerBaseWithConstantOffset

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. Additionally, instead of asserting later if the result
is > 64 bits, it breaks out of the loop in that case.

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

This commit must be merged after D38662 in order for the test to pass.

Patch by Michael Ferguson <mpfergu at gmail.com>.

Reviewers: reames, sanjoy, hfinkel

Reviewed By: hfinkel

Differential Revision: https://reviews.llvm.org/D38501

Added:
    llvm/trunk/test/Analysis/ValueTracking/gep-negative-issue.ll
Modified:
    llvm/trunk/lib/Analysis/ValueTracking.cpp

Modified: llvm/trunk/lib/Analysis/ValueTracking.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ValueTracking.cpp?rev=350395&r1=350394&r2=350395&view=diff
==============================================================================
--- llvm/trunk/lib/Analysis/ValueTracking.cpp (original)
+++ llvm/trunk/lib/Analysis/ValueTracking.cpp Fri Jan  4 06:53:22 2019
@@ -3389,7 +3389,14 @@ Value *llvm::GetPointerBaseWithConstantO
       if (!GEP->accumulateConstantOffset(DL, GEPOffset))
         break;
 
-      ByteOffset += GEPOffset.getSExtValue();
+      APInt OrigByteOffset(ByteOffset);
+      ByteOffset += GEPOffset.sextOrTrunc(ByteOffset.getBitWidth());
+      if (ByteOffset.getMinSignedBits() > 64) {
+        // Stop traversal if the pointer offset wouldn't fit into int64_t
+        // (this should be removed if Offset is updated to an APInt)
+        ByteOffset = OrigByteOffset;
+        break;
+      }
 
       Ptr = GEP->getPointerOperand();
     } else if (Operator::getOpcode(Ptr) == Instruction::BitCast ||

Added: llvm/trunk/test/Analysis/ValueTracking/gep-negative-issue.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/ValueTracking/gep-negative-issue.ll?rev=350395&view=auto
==============================================================================
--- llvm/trunk/test/Analysis/ValueTracking/gep-negative-issue.ll (added)
+++ llvm/trunk/test/Analysis/ValueTracking/gep-negative-issue.ll Fri Jan  4 06:53:22 2019
@@ -0,0 +1,43 @@
+; 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"
+
+%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 }
+%_array = type { i64, %ArrayImpl addrspace(100)*, i8 }
+
+define void @test(i64 %n_chpl) {
+entry:
+  ; First section is some code
+  %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
+  ; Second section is that code repeated
+  %x0 = getelementptr inbounds %_array, %_array* null, i32 0, i32 1
+  %x1 = load %ArrayImpl addrspace(100)*, %ArrayImpl addrspace(100)** %x0
+  %x2 = getelementptr inbounds %ArrayImpl, %ArrayImpl addrspace(100)* %x1, i32 0, i32 8
+  %x3 = load double addrspace(100)*, double addrspace(100)* addrspace(100)* %x2
+  %x4 = getelementptr inbounds double, double addrspace(100)* %x3, i64 -1
+  ; Stores that should be to the same location
+  store double 0.000000e+00, double addrspace(100)* %4
+  store double 0.000000e+00, double addrspace(100)* %x4
+  ; Third section is the repeated code again, with a later store
+  ; This third section is necessary to trigger the crash
+  %y1 = load %ArrayImpl addrspace(100)*, %ArrayImpl addrspace(100)** %0
+  %y2 = getelementptr inbounds %ArrayImpl, %ArrayImpl addrspace(100)* %y1, i32 0, i32 8
+  %y3 = load double addrspace(100)*, double addrspace(100)* addrspace(100)* %y2
+  %y4 = getelementptr inbounds double, double addrspace(100)* %y3, i64 -1
+  store double 0.000000e+00, double addrspace(100)* %y4
+  ret void
+; CHECK-LABEL: define void @test
+; CHECK: getelementptr inbounds double, double addrspace(100)* {{%.*}}, i64 -1
+; CHECK-NEXT: store double 0.000000e+00, double addrspace(100)* [[DST:%.*]]
+; CHECK-NEXT: store double 0.000000e+00, double addrspace(100)* [[DST]]
+; CHECK: load
+; CHECK: getelementptr inbounds %ArrayImpl, %ArrayImpl addrspace(100)*
+; CHECK: load
+; CHECK: getelementptr inbounds double, double addrspace(100)* {{%.*}}, i64 -1
+; CHECK: store double 0.000000e+00, double addrspace(100)*
+; CHECK: ret
+}