[PATCH] [CodeGen] Combine concat_vectors of scalars into build_vector.

[Chandler Carruth]

Bunch of nit-picky comments, but feel free to submit this once addressed. None of this seems terribly fundamental or important, and the transform seems quite good.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11510
@@ +11509,3 @@
+
+  SDLoc dl(N);
+  EVT VT = N->getValueType(0);
----------------
Since this is essentially a new function, please use 'DL' to be consistent with the coding conventions.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11516-11524
@@ +11515,11 @@
+  for (const SDValue &Op : N->ops()) {
+    if (ISD::BITCAST == Op.getOpcode() &&
+        !Op.getOperand(0).getValueType().isVector()) {
+      NumFPOps += Op.getOperand(0).getValueType().isFloatingPoint();
+      Ops.push_back(Op.getOperand(0));
+    } else if (ISD::UNDEF == Op.getOpcode()) {
+      Ops.push_back(Op);
+    } else {
+      return SDValue();
+    }
+  }
----------------
Are integer vectors the common case here? I would somewhat imagine they are as they seem the most likely to be widened or narrowed, but I've no idea what tests you're looking at. The rest assumes the default should be integer, but invert it appropriately if that's not the case.

I would construct a fresh UNDEF node here with the integer type so that if we don't need to cast to floating point, we can just skip ahead.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11527-11528
@@ +11526,4 @@
+
+  // Cast the scalars to either float or integer, depending on the most common
+  // one in the operands.
+  EVT SVT = (NumFPOps > N->getNumOperands() / 2)
----------------
I'm not sure this is the right tactic. If we have a mixture of floating point and integer inputs, I think we should assume the entire vector is intended to be floating point. We don't form floating point vectors randomly anywhere, but the integers could come from generic loads or some such that had no typed operation. Does that make sense to you? It also simplifies the logic -- you can track whether we see any non-floating-point operands and any floating point operands, and if we see both, bitcast all the inputs.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11532-11534
@@ +11531,5 @@
+                : EVT::getIntegerVT(*DAG.getContext(), OpVT.getSizeInBits());
+  std::for_each(Ops.begin(), Ops.end(), [&](SDValue &Op) {
+    Op = DAG.getNode(ISD::BITCAST, dl, SVT, Op);
+  });
+
----------------
Why std::for_each rather than a range based for loop?

I would also test the type of the op before bitcasting it to avoid the (sadly non-trivial) machinery involved in constantly folding no-op bitcasts.

http://reviews.llvm.org/D8948

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/