[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