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

[Eric Christopher via Phabricator via llvm-commits]

echristo added a comment.

Few more inline comments :)



================
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);
----------------
These need documenting as to what each of them means.


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


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2511
+    return false;
+  else
+    WideRes = getSETCCInGPR(N->getOperand(0), ConvOpts);
----------------
No need for an else here. Can just leave it as an early return.


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


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


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


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2524
+
+  if (N->getOpcode() == ISD::SIGN_EXTEND)
+    NumSextSetcc++;
----------------
Can probably hoist the N->getOptcode() == ISD::SIGN_EXTEND up to the beginning of the function.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2561
+  SignExtensionsAdded++;
+  return SDValue(CurDAG->getMachineNode(PPC::EXTSW_32, dl, MVT::i32, Input), 0);
+}
----------------
May wish to assert that the input is MVT:i32?


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2568-2569
+SDValue PPCDAGToDAGISel::zeroExtendInputIfNeeded(SDValue Input) {
+  ConstantSDNode *InputConst = dyn_cast<ConstantSDNode>(Input);
+  bool InputZExtConst = InputConst && InputConst->getSExtValue() >= 0;
+  LoadSDNode *InputLoad = dyn_cast<LoadSDNode>(Input);
----------------
Let's go ahead and sink these two down to where they're used.


================
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)),
----------------
Did we want an i32 assertion in this function as well?


================
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 {
----------------
Probably want to do something like:

if (cond) {
  <stuff>
  return SDValue(...)
}

assert(otherCond);
<stuff>
return SDValue(...);


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


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2685
+  EVT InputVT = LHS.getValueType();
+  if (InputVT != MVT::i32 && InputVT != MVT::i64)
+    return SDValue();
----------------
Do you mean '||' maybe?


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


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4810
 }
+//===-- PPCISelDAGToDAG.cpp - PPC --pattern matching inst selector --------===//
----------------
Cut and paste? :)


Repository:
  rL LLVM

https://reviews.llvm.org/D31847