This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] fold insertelement of scalar constant into vector of constants
ClosedPublic

Authored by spatel on Mar 21 2017, 10:58 AM.

Details

Summary

Keeping the various operand names straight for a pair of insertelements is challenging. I've reused the existing code in the patch, but if I renamed those code variables, it might be something like this:
insertelement (insertelement VectorC, X, Idx1C), ScalarC, Idx0C --> insertelement NewVectorC, X, Idx1C

So we're putting the scalar constant from the 2nd instruction into the vector constant operand of the 1st instruction because that eliminates the 2nd insertelement.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Mar 21 2017, 10:58 AM
efriedma edited edge metadata.Mar 21 2017, 11:22 AM

Would it make sense for this transform to look through more than one insertelement? Alternatively, we could perform this transform unconditionally: transform (insertelement (insertelement, X, Y, IdxC1), C, IdxC2) -> (insertelement (insertelement X, C, IdxC2), Y, IdxC1), ignoring the actual value of X.

lib/Transforms/InstCombine/InstCombineVectorOps.cpp
745 ↗(On Diff #92511)

Reusing Val[1] like this is confusing; can you use a separate variable?

758 ↗(On Diff #92511)

getAggregateElement can return null.

spatel marked 2 inline comments as done.Mar 21 2017, 2:08 PM

Alternatively, we could perform this transform unconditionally: transform (insertelement (insertelement, X, Y, IdxC1), C, IdxC2) -> (insertelement (insertelement X, C, IdxC2), Y, IdxC1), ignoring the actual value of X.

That looks great - we canonicalize harder, the constant folding will happen via the Builder, and it makes the patch cleaner.

spatel updated this revision to Diff 92557.Mar 21 2017, 3:14 PM

Patch updated as suggested by Eli:
Canonicalize an insert of a constant before an insert of a variable. There was already a test for this (but did nothing before), so I added a comment to explain.

efriedma accepted this revision.Mar 21 2017, 4:24 PM

LGTM.

(I guess this algorithm is theoretically quadratic over a long sequence of insertelements, but the constant factor is small enough that it shouldn't matter.)

This revision is now accepted and ready to land.Mar 21 2017, 4:24 PM
This revision was automatically updated to reflect the committed changes.