[PATCH] Add DAG combine for shl + add of constants.

[Andrea Di Biagio]

Hi Matt,

I have a couple of comments about your patch (see below).

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4146-4157
@@ -4145,2 +4145,14 @@
 
+  // fold (shl (add x, c1), c2) -> (add (shl x, c2), c1 << c2)
+  // Variant of version done on multiply, except mul by a power of 2 is turned
+  // into a shift.
+  APInt Val;
+  if (N1C && N0.getOpcode() == ISD::ADD && N0.getNode()->hasOneUse() &&
+      (isConstantSplatVector(N0.getOperand(1).getNode(), Val) ||
+       isa<ConstantSDNode>(N0.getOperand(1)))) {
+    SDValue Shl0 = DAG.getNode(ISD::SHL, SDLoc(N0), VT, N0.getOperand(0), N1);
+    SDValue Shl1 = DAG.getNode(ISD::SHL, SDLoc(N1), VT, N0.getOperand(1), N1);
+    return DAG.getNode(ISD::ADD, SDLoc(N), VT, Shl0, Shl1);
+  }
+
   if (N1C) {
----------------
If I understand correctly, your rule doesn't try to fold the case where C2 is a build_vector of all ConstantSDNode or Undef. Is this intentional?
If so, then why at line 4151 you check for 'isConstantSplatVector'? I am asking this because you specifically check for N1C; however, (unless I misread the code) N1C is not null only if N1 is a ConstantSDNode.

Also, I noticed that the dag combiner already implements a similar (but less poweful) rule in 'visitADD' at around line 1658 (see also function 'combineShlAddConstant'); the rule is:
// fold (add (shl (add x, c1), c2), ) -> (add (add (shl x, c2), c1<<c2), )

I think that your patch would make that rule redundant. I would do an experiment and see if that is true; in case, I suggest to get rid of it (and also the static function 'combineShlAddConstant').

I hope this helps.
Andrea

http://reviews.llvm.org/D4768