[PATCH] D25221: [PPC][DAGCombine] Convert SETCC to subtract when the result is zero extended

[Nemanja Ivanovic via llvm-commits]

nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:9891
 
+static SDValue GenerateEquivalentSub(SDNode *N, int size, bool complement,
+                                     bool swap, SDLoc &dl, SelectionDAG &DAG) {
----------------
I'm not sure why the variable names are lowercase here.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:9893
+                                     bool swap, SDLoc &dl, SelectionDAG &DAG) {
+  auto Op0 = DAG.getNode(ISD::ZERO_EXTEND, dl, MVT::i64, N->getOperand(0),
+                         DAG.getConstant(size, dl, MVT::i32));
----------------
I think some sort of assert is in order here to ensure this function is called with the correct type of node.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:9903
+  auto Shifted = DAG.getNode(ISD::SRL, dl, MVT::i64, SubNode,
+                             DAG.getConstant(size - 1, dl, MVT::i32));
+  auto Final = Shifted;
----------------
Could the expression `size - 1` ever be zero, thereby making this a redundant shift operation?


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:9917
+
+  if (!DCI.isAfterLegalizeVectorOps())
+    return SDValue();
----------------
Perhaps a comment describing why it's not valid to do this prior to legalization.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:9928
+
+  ISD::CondCode CC = cast<CondCodeSDNode>(N->getOperand(2))->get();;
+  auto OpSize = N->getOperand(0).getNode()->getValueType(0).getSizeInBits();
----------------
I think an assert should be added somewhere in this function as well to make sure you have the right node type or else this may trip an assert in a location that will make it difficult to debug.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:9929
+  ISD::CondCode CC = cast<CondCodeSDNode>(N->getOperand(2))->get();;
+  auto OpSize = N->getOperand(0).getNode()->getValueType(0).getSizeInBits();
+
----------------
Do we really need this much indirection? Is there not a `getValueType() `member function in the `SDValue` operand? Will `SDValue::getValueSizeInBits()` not produce the right value?


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:9931
+
+  unsigned size = 64; //size of widest legal int on PPC
+
----------------
Shouldn't there be a check somewhere to ensure we're not in 32-bit mode?


https://reviews.llvm.org/D25221