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

[Ehsan Amiri via llvm-commits]

amehsan 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) {
----------------
kbarton wrote:
> nemanjai wrote:
> > I'm not sure why the variable names are lowercase here.
> Need comments here about the logic and what the assumptions are in terms of input parameters, etc. 
Will fix lower case var names.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:9891
 
+static SDValue GenerateEquivalentSub(SDNode *N, int size, bool complement,
+                                     bool swap, SDLoc &dl, SelectionDAG &DAG) {
----------------
amehsan wrote:
> kbarton wrote:
> > nemanjai wrote:
> > > I'm not sure why the variable names are lowercase here.
> > Need comments here about the logic and what the assumptions are in terms of input parameters, etc. 
> Will fix lower case var names.
Will add comments.


================
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));
----------------
nemanjai wrote:
> I think some sort of assert is in order here to ensure this function is called with the correct type of node.
Will add


================
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;
----------------
nemanjai wrote:
> Could the expression `size - 1` ever be zero, thereby making this a redundant shift operation?
No, this is going to be the size of largest legal integer.


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


================
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();
----------------
kbarton wrote:
> nemanjai wrote:
> > 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.
> Some guidelines on the types of casts that can be used are here: http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates
> 
> It's probably good to pick one, and then either put the appropriate checks earlier, or make sure to add comments stating it is assumed this is only called with nodes of a specific type. 
Yes, this function is only called for ISD::SETCC. Will add more comments and assertions. 


================
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();
+
----------------
nemanjai wrote:
> 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?
Yes, I think `SDValue::getValueSizeInBits` will do what I need. 


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:9931
+
+  unsigned size = 64; //size of widest legal int on PPC
+
----------------
nemanjai wrote:
> Shouldn't there be a check somewhere to ensure we're not in 32-bit mode?
This should be taken from DataLayout object. Will fix that.


https://reviews.llvm.org/D25221