[PATCH] D27916: [RFC]Make the canonicalisation on shifts benifit to more case.

[Renato Golin via Phabricator via llvm-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

Thanks Jojo! I have a few nitpicks, but it's good to commit with those fixes.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4557
+      !isa<ConstantSDNode>(BinOpLHSVal->getOperand(1))) &&
+       BinOpLHSVal->getOpcode() != ISD::CopyFromReg &&
+       BinOpLHSVal->getOpcode() != ISD::SELECT)
----------------
rengolin wrote:
> This `if` is getting a bit bloated. I think we should create a few boolean variables with names.
> 
> Something like:
> 
>     auto Opc = BinOpLHSVal->getOpcode();
>     bool isShift = (Opc == ISD::SHL ...);
>     bool isConstantOp = isa<ConstantSDNode>(BinOpLHSVal->getOperand(1);
>     bool isCopyOrSelect = (Opc == ISD::CopyFromReg ...);
>     
>     if ((!isShift || !isConstantOp) && !isCopyOrSelect)
>       ...
> 
This is looking much better, thanks!


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4548
+  // FIXME: disable this unless the input to the binop is a shift by a constant
+  // or is copy/select.Enable this in other cases when figure out it's exactly profitable.
   SDNode *BinOpLHSVal = LHS->getOperand(0).getNode();
----------------
Is this still a FIXME? I think this comment could be just about what you're avoiding here, since it should have fixed the pessimized case, no?


================
Comment at: test/CodeGen/ARM/shift-combine.ll:6
+define i32 @test_lshr_and1(i32 %x) {
+;CHECK-LABEL: test_lshr_and1
+;CHECK-NOT lsr
----------------
It would be good, at least as documentation, to `CHECK` for the instructions you want to see. Same on the case below.


https://reviews.llvm.org/D27916