Page MenuHomePhabricator

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

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



Follow-up to

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.


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.


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


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


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!



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


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


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