Page MenuHomePhabricator

[InstSimplify] Fix SimplifyGEPInst when GEP index type is vector type.
Needs ReviewPublic

Authored by huihuiz on Mar 23 2020, 5:24 PM.

Details

Summary

The following folds

"gep (gep V, C), (sub 0, V) -> C"
"gep (gep V, C), (xor V, -1) -> C-1"

are only valid when GEP index type is scalar type.

This patch bailout early when GEP index type is vector type. Explicitly call
TypeSize::getFixedSize to assert on places where scalable type doesn't make sense.

Diff Detail

Event Timeline

huihuiz created this revision.Mar 23 2020, 5:24 PM

Take test.ll, run opt -S -instsimplify -o -
Then you will get the compiler warning: Compiler has made implicit assumption that TypeSize is not scalable.

define <vscale x 16 x i8*> @getelementptr_src_ty_not_vscale(<vscale x 16 x i8*> %a, <vscale x 16 x i64> %offset) {
  %v = getelementptr i8, <vscale x 16 x i8*> %a, <vscale x 16 x i64> %offset
  ret <vscale x 16 x i8*> %v
}

For this case SrcTy is i8

There are two different kinds of "scalable" involved when you're dealing with GEPs: whether you're indexing over a scalable type, and whether the result is a scalable vector of pointers. Mixing the two together into a single boolean doesn't really make sense.

In this context, I'm not sure why it would matter whether the result is a vector of pointers.

huihuiz updated this revision to Diff 257524.EditedApr 14 2020, 3:33 PM
huihuiz edited the summary of this revision. (Show Details)

My initial thought was wrong.
Spent sometime look again, we are nearly impossible to generate incorrect result without the code fixes in this patch.
But comparing a scalable size with a fixed size still doesn't make sense.

E.g., We are comparing scalable type size %offset to a fixed size 'IdxWidth' returned by DataLayout::getIndexSizeInBits

define <vscale x 1 x i8*> @gep(i8* %a, <vscale x 1 x i32> %offset) {
  %v = getelementptr i8, i8* %a, <vscale x 1 x i32> %offset
  ret <vscale x 1 x i8*> %v
}

Please help take a look, let me know what you think?

efriedma added inline comments.Apr 14 2020, 5:41 PM
llvm/lib/Analysis/InstructionSimplify.cpp
4153

This is supposed to be comparing the size of the scalar types, I think? Scalar types can't be scalable.

That said, it looks like the code isn't actually doing that at the moment.

huihuiz updated this revision to Diff 258113.Apr 16 2020, 11:40 AM
huihuiz marked an inline comment as done.
huihuiz retitled this revision from [InstSimplify][SVE] Minor fix SimplifyGEPInst for scalable vector. to [InstSimplify] Fix SimplifyGEPInst when GEP index type is vector type..
huihuiz edited the summary of this revision. (Show Details)
huihuiz added reviewers: spatel, majnemer.

Thanks Eli for the feedback!

Update the patch to address the folds in question, reject if GEP index type is vector type.

efriedma added inline comments.Apr 16 2020, 1:43 PM
llvm/lib/Analysis/InstructionSimplify.cpp
4143

I think the transform is still valid, it just isn't implemented.