[PATCH] D14496: X86: More efficient codegen for 64-bit compares on 32-bit target

[David Kreitzer via llvm-commits]

DavidKreitzer added inline comments.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2749
@@ +2748,3 @@
+
+  if (TLI.getOperationAction(ISD::SETCC_PARTS, LHSLo.getValueType()) ==
+      TargetLowering::Custom) {
----------------
hans wrote:
> DavidKreitzer wrote:
> > This looks reasonable to me.
> > 
> > I am just wondering whether it would be better to refactor this such that we always generate SETCC_PARTS here regardless of whether the target has a custom lowering and then implement a default lowering that does most of what this routine is currently doing.
> > 
> Can you provide an example of how always generating SETCC_PARTS and having a default lowering would help? One problem I see is that we'd need to legalize it too.
> 
> For example a 128-bit SETCC could be legalized to 64-bit SETCC_PARTS, and on a 32-bit target we'd need to legalize that too. With the current patch, we only legalize SETCC to SETCC_PARTS for types the target has said it can efficiently lower.
I didn't have any particular example in mind.  I was just thinking about alternative design choices. The current SETCC_PARTS design seems like it is putting a little too much target-specific knowledge in this target-independent piece of type legalization.

Another option you might like that is in line with your earlier thinking, i.e.

> I did try to come up with a way to make this more generic, using ISD::SUBC and SUBE

is to define a new SETCCE opcode that behaves just like SETCC except that it has an additional borrow operand. So the sequence you'd generate during type legalization is

   SUBC LHSLo, RHSLo
   SETCCE LHSHi, RHSHi, <SUBC borrow>, <cond>

And then the SETCCE would be implemented using SBB+SETcc rather than CMP+SETcc as SETCC is currently implemented.

It is probably easier and/or more natural to extend SETCCE to other targets than SETCC_PARTS.

What do you think?



http://reviews.llvm.org/D14496