Page MenuHomePhabricator

[ConstantFold] Support vector index when factoring out GEP index into preceding dimensions
ClosedPublic

Authored by haicheng on Nov 2 2017, 8:28 AM.

Details

Summary

Follow-up of D38677. This patch supports the vector type of both current and previous index when factoring out the current one into the previous one.

Diff Detail

Repository
rL LLVM

Event Timeline

haicheng created this revision.Nov 2 2017, 8:28 AM

Kindly Ping

Kindly Ping (#2)

mssimpso added inline comments.Nov 17 2017, 11:09 AM
lib/IR/ConstantFold.cpp
2214

You should be able to just use !isa<Constant>(Idxs[0]) for these kinds of checks I think. All GEP indices are required to be integers or integer vectors.

2250

This should be an else instead of an else if. Idxs[i] should be a vector here, so just cast it without checking.

2253

Use auto.

2280–2281

Use auto

2286–2291

This is confusing; I would get rid of it. The zero value is never used either. I would just use PrevIdx->getType->getNumVectorElements() or the equivalent for CurrIdx whenever you need IdxNumElements (see below for an example).

2294

CurrIdx = ConstantDataVector::getSplat(PrevIdx->getType()->getNumVectorElements(), CurrIdx);

2297

PrevIdx = ConstantDataVector::getSplat(CurrIdx->getType()->getNumVectorElements(), PrevIdx);

haicheng updated this revision to Diff 123525.Nov 19 2017, 6:24 PM
haicheng marked 6 inline comments as done.

Address most of Matt's comments. Thank you.

haicheng added inline comments.Nov 19 2017, 6:32 PM
lib/IR/ConstantFold.cpp
2214

I think Unknown is set to true if we don't know the indices are inbound or not. Since we only check the bound for ConstantInt and ConstantDataVector, I just white list these two. It is true that the constant index can only be constant integer and constant integer vector, but wild constant expression (e.g. ptrtoint) is also allowed. If I change the code to !isa<Constant>, many test cases fail.

mssimpso edited edge metadata.Dec 1 2017, 12:23 PM

One minor comment. Otherwise, this looks fine to me.

lib/IR/ConstantFold.cpp
2251

Shouldn't this be an else with an unchecked cast?

else {
  auto *CV = cast<ConstantDataVector>(Idxs[i]);
  ...
}
haicheng updated this revision to Diff 125261.Dec 2 2017, 5:16 AM
haicheng marked an inline comment as done.

Address Matt's comments. Thank you.

This revision was automatically updated to reflect the committed changes.