[PATCH] D25221: [PPC][DAGCombine] Convert SETCC to subtract when the result is zero extended
Nemanja Ivanovic via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 13 15:31:57 PDT 2016
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
More information about the llvm-commits
mailing list