This is an archive of the discontinued LLVM Phabricator instance.

[x86] Refactor the CMOV conversion pass to be more flexible.
ClosedPublic

Authored by chandlerc on Aug 16 2017, 12:45 AM.

Details

Summary

The primary thing that this accomplishes is to allow future re-use of
these routines in more contexts and clarify the behavior w.r.t. loops.
For example, if handling outer loops is desirable, doing so in
a inside-out order becomes straight forward because it walks the loop
nest itself (rather than walking the function's basic blocks) and
de-couples the CMOV rewriting from the loop structure as there isn't
actually anything loop-specific about this transformation.

This patch should be essentially a no-op. It potentially changes the
order in which we visit the inner loops, but otherwise should merely set
the stage for subsequent changes.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc created this revision.Aug 16 2017, 12:45 AM
aaboud edited edge metadata.Aug 16 2017, 7:22 AM

The code looks good, just few comments need to be fixed/added (see below)

lib/Target/X86/X86CmovConversion.cpp
100 ↗(On Diff #111315)

need to fix the "param" comment.

109 ↗(On Diff #111315)

need to fix the "param" comment.

169 ↗(On Diff #111315)

You are calling "Loops.size()" every iteration intentionally, as you want to handle the just new added sub-loops.
Do not you think we should comment on that, so nobody will optimize this by saving the initial "size()" once and check against it later?

chandlerc updated this revision to Diff 111604.Aug 17 2017, 7:00 PM
chandlerc marked 3 inline comments as done.

Improve comments based on code review.

Thanks for the review, updated!

lib/Target/X86/X86CmovConversion.cpp
169 ↗(On Diff #111315)

Sure.

aaboud accepted this revision.Aug 18 2017, 3:30 AM

LGTM.

This revision is now accepted and ready to land.Aug 18 2017, 3:30 AM
This revision was automatically updated to reflect the committed changes.