This is an archive of the discontinued LLVM Phabricator instance.

[X86] Special-case 2x CMOV when custom-inserting.
ClosedPublic

Authored by ab on Mar 2 2015, 2:28 PM.

Details

Reviewers
MatzeB
Summary

Follow-up to http://reviews.llvm.org/D7634.

This lets us avoid a few copies that are otherwise hard to get rid of. My first approach was to add a CMOV2 node, but after discussing with Matthias, we agreed it's less hacky to teach the custom inserter to look at the next instruction. Mentioned that explicitly in the EmitWithCustomInserter comment.

-Ahmed

Diff Detail

Event Timeline

ab updated this revision to Diff 21041.Mar 2 2015, 2:28 PM
ab retitled this revision from to [X86] Special-case 2x CMOV when custom-inserting..
ab updated this object.
ab edited the test plan for this revision. (Show Details)
ab added a reviewer: MatzeB.
ab added a subscriber: Unknown Object (MLST).
ab updated this revision to Diff 21044.Mar 2 2015, 2:37 PM
  • Avoid duplicating the "has double CMOV" information.
MatzeB accepted this revision.Mar 2 2015, 3:53 PM
MatzeB edited edge metadata.

This approach definitely looks better than the previous CMOV2 attempts.
I think the code should have more comments explaining why the special case for two cmovs is good here. Maybe put in the control flow ascii-art? as the problem is very subtle.

With more comments this LGTM, nitpicks below.

lib/Target/X86/X86ISelLowering.cpp
18117

I'm probably missing something, but it seems unnecessary to me to open a new block.

18118–18119

MachineBasicBlock::iterator InstrIt = std::next(MI); ? I'd also choose a name like NextMIIt;

18143–18147

Why not simply:
MachineInstr *LastEFlagsUser = NextCMOV ? NextCMOV : MI;
bool EflagsKilled = MI->killsRegister(X86::EFLAGS) || checkAndUpdateEFLAGSKill(LastEFLagsUser, BB, TRI));
if (!EflagsKilled) ...

This revision is now accepted and ready to land.Mar 2 2015, 3:53 PM
ab added a comment.Mar 2 2015, 4:23 PM

Thanks for the review, will commit shortly!

-Ahmed

lib/Target/X86/X86ISelLowering.cpp
18117

Unnecessary yes, originally to hide the short-lived declarations. I'll drop it.

18118–18119

I don't think that works. Since MI is a pointer, I'd expect that to be the same as

(MI + 1)

which is different from

&*(MBB::iterator(MI) + 1)
ab closed this revision.Mar 2 2015, 5:29 PM

r231046.

Maybe I'm not patient, but phabricator also doesn't like "Closes". "Differential Revision" it is, then!

-Ahmed