Page MenuHomePhabricator

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

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



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


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
2214 ↗(On Diff #121319)

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 ↗(On Diff #121319)

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

2253 ↗(On Diff #121319)

Use auto.

2279–2280 ↗(On Diff #121319)

Use auto

2285–2290 ↗(On Diff #121319)

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).

2293 ↗(On Diff #121319)

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

2296 ↗(On Diff #121319)

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
2214 ↗(On Diff #121319)

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.

2251 ↗(On Diff #123525)

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.