This is an archive of the discontinued LLVM Phabricator instance.

Check for overflows when calculating the offset in GetGEPCost.
ClosedPublic

Authored by jlebar on Sep 27 2017, 3:19 PM.

Details

Summary

This avoids C++ UB if the GEP is weird and the calculation overflows
int64_t, and it's also observable in the cost model's results.

Such GEPs are almost surely not valid pointers, but LLVM nonetheless
generates them sometimes.

Event Timeline

jlebar created this revision.Sep 27 2017, 3:19 PM
efriedma added inline comments.
llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
681

Should we use the pointer width here, rather than the constant "64"?

705

Might as well use wrapping math, rather than bailing out on overflow; that's how the actual lowering works.

jlebar updated this revision to Diff 116898.Sep 27 2017, 3:54 PM
jlebar marked 2 inline comments as done.

Wrap the GEP indices instead of bailing on overflow.

Thank you for the review, Eli.

llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
681

Looking at the langref, I think you're right, here and below. I've updated the patch, wdyt?

efriedma added inline comments.Sep 27 2017, 4:04 PM
llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
707

ConstIdx->getValue().sextOrTrunc(PtrSizeBits)? getSExtValue() can fail if ConstIdx is, for example, an i128.

jlebar updated this revision to Diff 116899.Sep 27 2017, 4:14 PM
jlebar marked an inline comment as done.

Call sextOrTrunc instead of assuming the offset constant has the same width as the pointer.

llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
707

Indeed. Fixed, and added a testcase.

This revision is now accepted and ready to land.Sep 27 2017, 4:17 PM
This revision was automatically updated to reflect the committed changes.
eastig reopened this revision.Oct 4 2017, 6:05 AM
eastig added a subscriber: eastig.

Hi Justin,

This patch has an problem with negative offsets and 32-bit pointers.
Here is a test case:

target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
target triple = "thumbv7em-arm-none-eabi"

define internal void @test(i16* %pOut, i16* %pIn) {
entry:
  br label %for.body

for.body:                                         ; preds = %entry, %for.body
  %pIn.pn45 = phi i16* [ %pIn, %entry ], [ %add.ptr9, %for.body ]
  %i.046 = phi i32 [ 0, %entry ], [ %inc, %for.body ]
  %pIn.addr.0 = getelementptr inbounds i16, i16* %pIn.pn45, i32 -32
  %0 = load i16, i16* %pIn.pn45, align 2
  store volatile i16 %0, i16* %pOut, align 2
  %add.ptr9 = getelementptr inbounds i16, i16* %pIn.pn45, i32 -64
  %inc = add nsw i32 %i.046, 1
  %cmp = icmp slt i32 %inc, 21
  br i1 %cmp, label %for.body, label %for.end

for.end:                                          ; preds = %for.body
  ret void
}

In armv7m the pointer size is 32-bit. "ConstIdx->getValue().sextOrTrunc(PtrSizeBits) " sets the width to 32. "BaseOffset.getLimitedValue()" calls "APInt::getZExtValue()" which returns a zero extended value. So a negative value is truncated to 32-bit and then zero extended to uint64_t. As a result a wrong value is created.

Run the following in a debugger to reproduce:

opt -loop-unswitch -S test.ll

Thanks,
Evgeny Astigeevich

This revision is now accepted and ready to land.Oct 4 2017, 6:05 AM

Thank you for the detailed bug report, Evgeny, and I'm sorry for the breakage.

I'm able to reproduce, and I've made a failing testcase for Analysis/CostModel/ARM. I'm working on the fix now.

Fix is out for review in D38557.

eastig closed this revision.Oct 5 2017, 1:52 AM

Thank you, Justin.