From 7902405c42ffe37f7e862dc9e442278394dec0c2 Mon Sep 17 00:00:00 2001 From: Florian Hahn Date: Fri, 4 Jan 2019 14:53:22 +0000 Subject: [PATCH] [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 . Reviewers: reames, sanjoy, hfinkel Reviewed By: hfinkel Differential Revision: https://reviews.llvm.org/D38501 llvm-svn: 350395 --- llvm/lib/Analysis/ValueTracking.cpp | 9 +++- .../ValueTracking/gep-negative-issue.ll | 43 +++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 llvm/test/Analysis/ValueTracking/gep-negative-issue.ll diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp index efbab47406c4..3c4cf1d88f9b 100644 --- a/llvm/lib/Analysis/ValueTracking.cpp +++ b/llvm/lib/Analysis/ValueTracking.cpp @@ -3389,7 +3389,14 @@ Value *llvm::GetPointerBaseWithConstantOffset(Value *Ptr, int64_t &Offset, 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 || diff --git a/llvm/test/Analysis/ValueTracking/gep-negative-issue.ll b/llvm/test/Analysis/ValueTracking/gep-negative-issue.ll new file mode 100644 index 000000000000..5ed7e068e317 --- /dev/null +++ b/llvm/test/Analysis/ValueTracking/gep-negative-issue.ll @@ -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 +}