Page MenuHomePhabricator

[ConstantFold] Fix a crash when folding a GEP that has vector index
ClosedPublic

Authored by haicheng on Oct 8 2017, 3:05 PM.

Details

Summary

LLVM crashes with following IRs if compiling it with opt -instsimplify. See PR34880

@block = global [64 x [8192 x i8]] zeroinitializer, align 1

define <2 x i8*> @foo() {
  %1 = getelementptr inbounds [64 x [8192 x i8]], [64 x [8192 x i8]]* @block, i64 0, <2 x i64> <i64 0, i64 1>, i64 8192
  ret <2 x i8*> %1
}

ConstantFolding tries to factor out the last index (8192) into the second last dimension. The code assumes the current index (8192) and the previous index (<i64 0, i64 1>) are both integer typed, but it now only checks if the current index is a ConstantInt. This patch exits early if the previous index is not a ConstantInt.

Diff Detail

Repository
rL LLVM

Event Timeline

haicheng created this revision.Oct 8 2017, 3:05 PM
majnemer added inline comments.Oct 8 2017, 3:16 PM
lib/IR/ConstantFold.cpp
2213 ↗(On Diff #118174)

Shouldn't this be !isa<Constant> ?

haicheng added inline comments.Oct 8 2017, 4:34 PM
lib/IR/ConstantFold.cpp
2213 ↗(On Diff #118174)

Using !isa<Constant> still crashes the test case. In the test case, when Idxs[i-1] is <i64 0, i64 1> which is a Constant but not a ConstantInt, code below will cast its type to IntegerType and then llvm crashes.

haicheng added inline comments.Oct 11 2017, 1:30 PM
lib/IR/ConstantFold.cpp
2259 ↗(On Diff #118174)

The crash happens here. Type::getIntegerBitWidth() casts underlying type to IntegerType and crashes when the underlying type is a vector.

2265 ↗(On Diff #118174)

I think this line also does not expect a vector PrevIdx.

2272 ↗(On Diff #118174)

I think this line also does not expect a vector PrevIdx.

Kindly Ping.

This patch just adds a simply check to bail out early to prevent LLVM from crashing.

mssimpso edited edge metadata.Oct 26 2017, 11:54 AM

Haicheng,

How hard do you think it would be to update this transformation to reason about vector GEPs rather than bailing out? (I've left some inline comments if you want to give it a try). Having said that, I'm fine with that patch as is. But you should at least add a comment explaining why we're giving up.

lib/IR/ConstantFold.cpp
2259 ↗(On Diff #118174)

getScalarScalarSizeInBits should work for both scalars and vectors.

2265 ↗(On Diff #118174)

isIntOrIntVectorTy should work for both scalars and vectors.

2272 ↗(On Diff #118174)

I believe the add should work for vectors as well, but if PrevIdx is a vector, I think you would have to broadcast Div to a vector.

haicheng added a comment.EditedOct 26 2017, 2:40 PM

Thank you, Matt.

As you pointed out, supporting vector previous index is not difficult. I choosed to bail out at first because

  1. We already bail out when the current index is a vector.
  1. I initially planned to fix the crash in this patch to unblock the release and add the vector index support for both current and previous index.
  1. I think this piece of algorithm work better if we iterate from the last index to the first. So, we don't need to recursively call this function in the following case.
@block = global [8 x [64 x [8192 x i8]]] zeroinitializer, align 1

define i8* @vectorindex() {
  %1 = getelementptr inbounds [8 x [64 x [8192 x i8]]], [8 x [64 x [8192 x i8]]]* @block, i64 0, i64 0, i64 63, i64 8192
  ret i8* %1
}
mssimpso accepted this revision.Oct 27 2017, 5:54 AM

Thank you, Matt.

As you pointed out, supporting vector previous index is not difficult. I choosed to bail out at first because

  1. We already bail out when the current index is a vector.
  2. I initially planned to fix the crash in this patch to unblock the release and add the vector index support for both current and previous index.
  3. I think this piece of algorithm work better if we iterate from the last index to the first. So, we don't need to recursively call this function in the following case.

    ` @block = global [8 x [64 x [8192 x i8]]] zeroinitializer, align 1

    define i8* @vectorindex() { %1 = getelementptr inbounds [8 x [64 x [8192 x i8]]], [8 x [64 x [8192 x i8]]]* @block, i64 0, i64 0, i64 63, i64 8192 ret i8* %1 } `

OK, that sounds good to me.

This revision is now accepted and ready to land.Oct 27 2017, 5:54 AM
haicheng updated this revision to Diff 120646.Oct 27 2017, 10:36 AM

Add a FIXME.

Thank you, Matt.

This revision was automatically updated to reflect the committed changes.