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

Nemanja Ivanovic via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue May 9 18:01:30 PDT 2017


nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2497
+bool PPCDAGToDAGISel::tryEXTEND(SDNode *N) {
+  if (TM.getOptLevel() == CodeGenOpt::None || !TM.isPPC64())
+    return false;
----------------
echristo wrote:
> The PPC64 bit is because of testing or some other reason?
We really only want to do this on 64-bit targets. The reason is that instructions sequences in many cases need to be different in 32-bit mode. This is a result of:
1. Registers not **actually** being 32 bits wide in 32-bit mode
2. The i64 values not fitting in single registers in 32-bit mode

I will add a comment to these early exits to this end.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:264-265
 private:
+    enum ExtOrTruncConversion { NoOp, Ext, Trunc };
+    enum SetccInGPROpts { ZExtOrig, ZExtInvert, SExtOrig, SExtInvert };
     bool trySETCC(SDNode *N);
----------------
echristo wrote:
> These need documenting as to what each of them means.
OK. I'll document them.


================
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.
----------------
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.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2511
+    return false;
+  else
+    WideRes = getSETCCInGPR(N->getOperand(0), ConvOpts);
----------------
echristo wrote:
> No need for an else here. Can just leave it as an early return.
Makes sense. Not sure why I left it in.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2512
+  else
+    WideRes = getSETCCInGPR(N->getOperand(0), ConvOpts);
+
----------------
echristo wrote:
> This can probably just be an if statement or a ternary operator in the call and get rid of ConvOpts as a local.
OK, I'll get rid of the temp.


================
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;
----------------
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.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2521
+    Inputs32Bit ? ExtOrTruncConversion::Ext : ExtOrTruncConversion::Trunc;
+  if (!WideRes)
+    return false;
----------------
echristo wrote:
> This can be a few lines up.
OK, makes sense.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2524
+
+  if (N->getOpcode() == ISD::SIGN_EXTEND)
+    NumSextSetcc++;
----------------
echristo wrote:
> Can probably hoist the N->getOptcode() == ISD::SIGN_EXTEND up to the beginning of the function.
We don't want to increment the stats in the early exit cases.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2561
+  SignExtensionsAdded++;
+  return SDValue(CurDAG->getMachineNode(PPC::EXTSW_32, dl, MVT::i32, Input), 0);
+}
----------------
echristo wrote:
> May wish to assert that the input is MVT:i32?
Will do. But at the beginning of the function. I'm not sure why the comment is at the end of the function.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2586
+  ZeroExtensionsAdded++;
+  return SDValue(CurDAG->getMachineNode(PPC::RLDICL_32, dl, MVT::i32, Input,
+                                        getI64Imm(0, dl), getI64Imm(32, dl)),
----------------
echristo wrote:
> Did we want an i32 assertion in this function as well?
Yup.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2604-2610
+  if (Conv == ExtOrTruncConversion::Ext) {
+    SDValue ImDef(CurDAG->getMachineNode(PPC::IMPLICIT_DEF, dl, MVT::i64), 0);
+    SDValue SubRegIdx =
+      CurDAG->getTargetConstant(PPC::sub_32, dl, MVT::i32);
+    ConvOp = SDValue(CurDAG->getMachineNode(PPC::INSERT_SUBREG, dl, MVT::i64,
+                                            ImDef, NatWidthRes, SubRegIdx), 0);
+  } else {
----------------
echristo wrote:
> Probably want to do something like:
> 
> if (cond) {
>   <stuff>
>   return SDValue(...)
> }
> 
> assert(otherCond);
> <stuff>
> return SDValue(...);
Yeah. When I refactored, I missed this weird setup in this function.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2683
+  ISD::CondCode CC =
+    cast<CondCodeSDNode>(Compare.getOperand(IsSelectCC ? 4 : 2))->get();
+  EVT InputVT = LHS.getValueType();
----------------
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;`


================
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)
----------------
echristo wrote:
> MVT InputVT
I thought we typically used the EVT class for this type of thing but sure.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2685
+  EVT InputVT = LHS.getValueType();
+  if (InputVT != MVT::i32 && InputVT != MVT::i64)
+    return SDValue();
----------------
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.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2690
+  ConstantSDNode *RHSConst = dyn_cast<ConstantSDNode>(RHS);
+  bool Inputs32Bit = InputVT == MVT::i32;
+  int64_t RHSValue = RHSConst ? RHSConst->getSExtValue() : INT64_MAX;
----------------
echristo wrote:
> Early returns if not 32bit inputs? It'll probably clean up the logic below as well. Also a bit confusing with the InputVT stuff above.
I suppose I can clean that up on this patch since it currently doesn't handle 64-bit inputs, but it'll just be right back as soon as I add it in one of the subsequent patches.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4810
 }
+//===-- PPCISelDAGToDAG.cpp - PPC --pattern matching inst selector --------===//
----------------
echristo wrote:
> Cut and paste? :)
That's bizarre. No idea how I did that. I'll of course remove it :).


Repository:
  rL LLVM

https://reviews.llvm.org/D31847





More information about the llvm-commits mailing list