[PATCH] D31847: [PowerPC] Eliminate compares in instruction selection - Vol. 1

[Eric Christopher via Phabricator via llvm-commits]

echristo accepted this revision.
echristo added a comment.
This revision is now accepted and ready to land.

Couple of small comment requests, but that's it.

Thanks!

-eric



================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2496-2498
+/// instructions and ISEL. We don't want to do this on 32-bit targets
+/// because the code relies on registers being 64-bits wide and i64
+/// values fitting in a single register.
----------------
nemanjai wrote:
> echristo wrote:
> > This sounds more like "This code is currently specialized to 64-bit registers" rather than "this transformation doesn't make sense on 32-bit" yes?
> Yes, the transformation as implemented is specialized for 64-bit targets. I'll update the comment.
"We only do this for 64-bit targets for now." maybe?


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2516-2520
+  bool Inputs32Bit = N->getOperand(0).getOperand(0).getValueType() == MVT::i32;
+  bool Output32Bit = N->getValueType(0) == MVT::i32;
+  ExtOrTruncConversion Conv =
+    Inputs32Bit == Output32Bit ? ExtOrTruncConversion::NoOp :
+    Inputs32Bit ? ExtOrTruncConversion::Ext : ExtOrTruncConversion::Trunc;
----------------
nemanjai wrote:
> echristo wrote:
> > Do we expect to do this with non-32bit quantities? It also seems that we don't do anything for NoOp and so can early return? (Reading down through addExtOrTrunc). Are you actually getting things like zext/sext 32->32? 
> The complete patch will cover all cases. Right now we really only do 32-bit comparisons and we may or may not want the value in a 64-bit register (depending on the result type). But eventually we'll have all the combinations:
> compare 32-bit values -> produce 32-bit value
> compare 32-bit values -> produce 64-bit value
> compare 64-bit values -> produce 32-bit value
> compare 64-bit values -> produce 64-bit value
> 
> Two of those don't require conversions, one requires an extension and the other a truncation.
Interesting. I'll just take this on faith :)


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2683
+  ISD::CondCode CC =
+    cast<CondCodeSDNode>(Compare.getOperand(IsSelectCC ? 4 : 2))->get();
+  EVT InputVT = LHS.getValueType();
----------------
nemanjai wrote:
> echristo wrote:
> > 4 and 2?
> Yup. 4 and 2. Just the operand numbers for the condition code in the two nodes. I'll rewrite this to name the operand number based on which node it is. Something like:
> `int CCOpNum = Compare.getOpcode() == ISD::SELECT_CC ? 4 : 2;`
Oh. Sure. That makes a lot more sense. Can you add a comment as well?


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2684
+    cast<CondCodeSDNode>(Compare.getOperand(IsSelectCC ? 4 : 2))->get();
+  EVT InputVT = LHS.getValueType();
+  if (InputVT != MVT::i32 && InputVT != MVT::i64)
----------------
nemanjai wrote:
> nemanjai wrote:
> > echristo wrote:
> > > MVT InputVT
> > I thought we typically used the EVT class for this type of thing but sure.
> On second thought, looks like it'll have to stay an EVT or else I'd have to call `getSimpleType()` on the result of `getValueType()`.
*shrug* Sure :)


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2685
+  EVT InputVT = LHS.getValueType();
+  if (InputVT != MVT::i32 && InputVT != MVT::i64)
+    return SDValue();
----------------
nemanjai wrote:
> echristo wrote:
> > Do you mean '||' maybe?
> Nope. Well I suppose I could DeMorgan-ize it to something like:
> 
> `if (!(InputVT == MVT::i32 || InputVT == MVT::i64))`
> but I don't find that any more readable.
> 
> Ultimately, I want to exit early if it isn't one of those two types.
k.


Repository:
  rL LLVM

https://reviews.llvm.org/D31847