[PATCH] D49966: [X86] Performing DAG pruning before selection of LEA instructions.
Roman Lebedev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Jul 29 13:54:03 PDT 2018
lebedev.ri added inline comments.
================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:705-707
+ SDLoc DL(N);
+ EVT VT = N->getValueType(0);
+ unsigned Opcode = N->getOpcode();
----------------
These can be moved to after the lambda.
================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:708
+ unsigned Opcode = N->getOpcode();
+ auto IsValidConstantShift = [=](SDValue Shift) {
+ SDNode *ShiftVal = Shift.getOperand(1).getNode();
----------------
But this doesn't check that it's *argument* is ValidConstantShift.
It checks that the `getOperand(1).getNode()` of the argument is `ValidConstantShift`.
How about you sink the `Opr0.getOpcode() == ISD::SHL` check into the lambda, and rename the lambda somehow?
================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:710-711
+ SDNode *ShiftVal = Shift.getOperand(1).getNode();
+ if (isa<ConstantSDNode>(ShiftVal) &&
+ (cast<ConstantSDNode>(ShiftVal)->getZExtValue() <= 8))
+ return true;
----------------
Is this sufficient to limit this to scalars only?
Also, should this be limited to only some specific `EVT`'s?
================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:716
+
+ if (Opcode == ISD::SUB) {
+ SDValue Opr0 = N->getOperand(0);
----------------
Are there other patterns with different root nodes possible? If not i'd early-return.
================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:717-718
+ if (Opcode == ISD::SUB) {
+ SDValue Opr0 = N->getOperand(0);
+ SDValue Opr1 = N->getOperand(1);
+ if (Opr0.getOpcode() == ISD::SHL && IsValidConstantShift(Opr0)) {
----------------
General naming comment: it seems, these would normally be named `N0` and `N1`, because they are 0'th and 1'th operands of `N`.
================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:744
+ }
+ }
+ return false;
----------------
Any thoughts on whether the `shl` should be one-use?
Repository:
rL LLVM
https://reviews.llvm.org/D49966
More information about the llvm-commits
mailing list