This is an archive of the discontinued LLVM Phabricator instance.

[LV] Extend trunc optimization to all IVs with constant integer steps
ClosedPublic

Authored by mssimpso on Feb 10 2017, 1:55 PM.

Details

Summary

This patch extends the optimization of truncations whose operand is an induction variable with a constant integer step. Previously we were only applying this optimization to the primary induction variable. However, the cost model assumes the optimization is applied to the truncation of all integer induction variables (even regardless of step type). The transform is now applied to the other induction variables, and I've updated the cost model to ensure it is better in sync with the transformation we actually perform.

Diff Detail

Repository
rL LLVM

Event Timeline

mssimpso created this revision.Feb 10 2017, 1:55 PM
mkuper accepted this revision.Feb 10 2017, 2:04 PM

LGTM

This revision is now accepted and ready to land.Feb 10 2017, 2:04 PM

Thanks Michael!

This revision was automatically updated to reflect the committed changes.

I'm reopening this review since I reverted the original patch. It caused execution time slowdowns in a few test-suite programs that were caught by the clang-cmake-aarch64-quick bot. It looks like the cause is related to the fact that we're not cost-modeling this optimization very well. Some truncations are free - if we replace those truncations with an induction variable, we'll end up with an extra instruction (the IV update) on every iteration of the loop. I'll post an updated patch shortly.

mssimpso reopened this revision.Feb 13 2017, 10:52 AM
This revision is now accepted and ready to land.Feb 13 2017, 10:52 AM
mssimpso updated this revision to Diff 88256.Feb 13 2017, 2:17 PM
  • I updated patch so that we extend the truncate optimization to non-primary induction variables only if the truncate isn't already free.
  • I factored out the logic into a helper so we can keep things in sync easier.
  • I added a test case for AArch64 showing that we keep a free truncate.

Heh, interesting.
Anyway, LGTM.

lib/Transforms/Vectorize/LoopVectorize.cpp
2037 ↗(On Diff #88256)

Unrelated, but after this lands, I think I'll change the name to getPrimaryInduction() or getPrimaryIV(). This is a really bad name at this point.

mssimpso added inline comments.Feb 14 2017, 6:40 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
2037 ↗(On Diff #88256)

I agree. Thanks Michael.

This revision was automatically updated to reflect the committed changes.