This is an archive of the discontinued LLVM Phabricator instance.

[X86] Remove x86ISD::INC/DEC. Just select them from X86ISD::ADD/SUB at isel time
ClosedPublic

Authored by craig.topper on Dec 20 2018, 5:52 PM.

Details

Summary

INC/DEC are pretty much the same as ADD/SUB except that they don't update the C flag.

This patch removes the special nodes and just pattern matches from ADD/SUB during isel if the C flag isn't being used.

I had to avoid selecting DEC is the result isn't used. This will become a SUB immediate which will turned into a CMP later by optimizeCompareInstr. This lead to the one test change where we use a CMP instead of a DEC for an overflow intrinsic since we only checked the flag.

This also exposed a hole in our RMW flag matching use of hasNoCarryFlagUses. Our root node for the match is a store and there's no guarantee that all the flag users have been selected yet. So hasNoCarryFlagUses needs to check copyToReg and machine opcodes, but it also needs to check for the pre-match SETCC, SETCC_CARRY, BRCOND, and CMOV opcodes.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Dec 20 2018, 5:52 PM

Remove the atomic INC/DEC ISD opcodes too.

RKSimon added inline comments.Dec 21 2018, 6:48 AM
lib/Target/X86/X86ISelDAGToDAG.cpp
2311 ↗(On Diff #179242)

(style) return true here for ISD::CopyToReg special case and remove else + indentation below?

craig.topper marked an inline comment as done.Dec 21 2018, 9:11 AM
craig.topper added inline comments.
lib/Target/X86/X86ISelDAGToDAG.cpp
2311 ↗(On Diff #179242)

We’re inside the outer for loop over users of the flags. We may have a mix of selected and unselected users. We can’t return true until we’ve visited them all.

RKSimon added inline comments.Dec 21 2018, 9:51 AM
lib/Target/X86/X86ISelDAGToDAG.cpp
2311 ↗(On Diff #179242)

Sorry, I'm miscounting braces.....

craig.topper marked an inline comment as done.Dec 21 2018, 10:26 AM
craig.topper added inline comments.
lib/Target/X86/X86ISelDAGToDAG.cpp
2311 ↗(On Diff #179242)

I'm going to add a continue there instead. And I'll move the duplicate CC switch into a helper.

Reduce code duplication and indendation in hasNoCarryFlagUses.

Merge some duplicate code in EmitTest. Remove now unused SDNodes from X86InstrInfo.td

I'm happy with this - @spatel @andreadb any comments?

spatel accepted this revision.Jan 2 2019, 7:34 AM

LGTM

lib/Target/X86/X86ISelLowering.cpp
18660 ↗(On Diff #179810)

instructions selection -> instruction selection ?
This is an awkward/long sentence, so it might be better to just edit:

// Transform to an x86-specific ALU node with flags if there is a chance of
// folding a memory operand or only the flags are used. Otherwise, leave 
// the node alone and emit a 'test' instruction.
This revision is now accepted and ready to land.Jan 2 2019, 7:34 AM
This revision was automatically updated to reflect the committed changes.