Page MenuHomePhabricator

[X86] Refactoring of X86TargetLowering::EmitLoweredSelect. nfc.
ClosedPublic

Authored by aivchenk on Jul 20 2017, 8:02 AM.

Details

Summary

Attempt to reduce the mess (276 lines of code in this function)
by putting the functionality of emitting cascaded selects into
a separate function and by extracting the common piece of code
(creating PHIs in the sink block).

Diff Detail

Repository
rL LLVM

Event Timeline

aivchenk created this revision.Jul 20 2017, 8:02 AM
aaboud edited edge metadata.Aug 1 2017, 6:15 AM

Few minor comments. below.
If I understood the code well, this is a non functional change, is that correct?

lib/Target/X86/X86ISelLowering.cpp
25596 ↗(On Diff #107512)

How about adding the "static" keyword?

25611 ↗(On Diff #107512)

You would need this comment before line 25608 above, as it explains the map define at that line.

25724 ↗(On Diff #107512)

[Style]: Any reason you break the comment line before reaching 80 characters?

25781 ↗(On Diff #107512)

Calling this helper function for one CMOV is not need (you will not be using half of the code that exist in the helper function), and it is even confusing.
You could simply create a single PHI node here and make the code much more readable.

25784 ↗(On Diff #107512)

[Style]: Please, use capital letter, "The second ..."

25927 ↗(On Diff #107512)

You may simply call:

ThisMBB->erase(MIItBegin, MIItEnd);
aivchenk updated this revision to Diff 109312.Aug 2 2017, 3:04 AM
aivchenk marked 6 inline comments as done.

Addressed Amjad's comments

Oh, and yes, it is not a functional change (I put "nfc" in the topic)

craig.topper added inline comments.Aug 10 2017, 3:39 PM
lib/Target/X86/X86ISelLowering.cpp
25612 ↗(On Diff #109312)

Maybe put a blank line before this return

25710 ↗(On Diff #109312)

The addLiveIn for jcc1MBB was only conditional on cascadedCMOV in the original code. Should it really be inside this if now?

25871 ↗(On Diff #109312)

Comment should say ThisMBB not ThiMBB

25879 ↗(On Diff #109312)

Space after 'always'.

aivchenk marked 3 inline comments as done.Aug 11 2017, 6:25 AM

Thanks for your comments, especially about addLiveIn one, I missed that

aivchenk updated this revision to Diff 110712.Aug 11 2017, 6:26 AM
aivchenk marked an inline comment as done.
craig.topper edited edge metadata.Aug 14 2017, 9:56 AM

This looks ok to me now. @RKSimon, do you want to look at this too?

RKSimon edited edge metadata.Aug 14 2017, 2:21 PM

LGTM too

RKSimon accepted this revision.Aug 15 2017, 2:59 AM
This revision is now accepted and ready to land.Aug 15 2017, 2:59 AM
This revision was automatically updated to reflect the committed changes.