Page MenuHomePhabricator

[X86] Rewrite LowerBRCOND to remove dead code and handle ISD::SETCC and overflow ops directly.
ClosedPublic

Authored by craig.topper on Feb 17 2020, 6:41 PM.

Details

Summary

There's a lot of old leftover code in LowerBRCOND. Especially
the detecting or AND or OR of X86ISD::SETCC nodes. Those were
needed before LegalizeDAG was changed to visit nodes before
their operands.

It also relied on reversing the output of LowerSETCC to find the
flags producing node to use for the X86ISD::BRCOND node.

Rather than using LowerSETCC this patch uses emitFlagsForSetcc to
handle the integer ISD::SETCC case. This gives the flag producer
and the comparison code to use directly. I've removed the addTest
flag and just produce a X86ISD::BRCOND and return immediately.

Floating point ISD::SETCC case is just an X86ISD::FCMP with special
care for OEQ and UNE derived from the previous code. I've left
f128 out so it will emit a test. And LowerSETCC will be called
later to produce a libcall and X86ISD::SETCC. We have combines
that can merge the test and X86ISD::SETCC.

We need to handle two cases for overflow ops. Either they are used
directly or they have a seteq 0 or setne 1 to invert the overflow.
The old code did not handle the setne 1 case, but I think some
other combines were making up for it.

If we fail to find a condition, we'll wrap an AND with 1 on the
original condition and tell emitFlagsForSetcc to emit a compare
with 0. This will pickup the LowerAndToBT and or the EmitTest case.
I kept the isTruncWithZeroHighBitsInput call, but we might be able
to fold that in to emitFlagsForSetcc.

Diff Detail

Event Timeline

craig.topper created this revision.Feb 17 2020, 6:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2020, 6:41 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
RKSimon added inline comments.Feb 18 2020, 3:55 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
23104–23105

Maybe put this in ISDOpcodes.h ?

craig.topper marked an inline comment as done.Feb 18 2020, 9:43 AM
craig.topper added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
23104–23105

The dependency on SDValue probably means it needs to go in SelectionDAGNodes.h?

RKSimon added inline comments.Feb 18 2020, 10:02 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
23104–23105

Sorry, I didn't notice the getResNo..........

Move isOverflowIntrOpRes to SelectionDAGNodes.h including AArch64 where I copied it from.

RKSimon accepted this revision.Feb 20 2020, 4:32 AM

LGTM, although it'd have been easier to grok in smaller changes tbh.

llvm/include/llvm/CodeGen/SelectionDAGNodes.h
2688 ↗(On Diff #245205)

pre-commit this and the AArch64ISelLowering.cpp change as an NFC.

This revision is now accepted and ready to land.Feb 20 2020, 4:32 AM
This revision was automatically updated to reflect the committed changes.