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

[Ahmed Bougacha]

================
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)
----------------
chandlerc wrote:
> 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.
I sat down and actually thought about this: I agree, if any of the inputs is floating point, all should be, but the real-world code I'm looking at doesn't involve floating point at all, so I might have the wrong reasons?

We can either look at:
- the vectors: floating point vectors is a strong hint the result type should really be floating point as well, and I think this is what you're saying above.
- the scalars: I see only one case where a floating point scalar value is "unintentionally" bitcast to a vector, and that's a target with say illegal i64 but legal f64 loads.  In that case, it's also a hint that we should prefer FP, but that's more because the integer scalar type is illegal than anything else, really.  When the IR does an "intentional" bitcast, the AArch64 "_mixed_" testcase shows another reason this is a good idea: moving the scalars to the vector/FP bank once is better (modulo micro-architectures) than doing all the insertions directly from the GPRs.

My conclusion is that if there's any floating point type anywhere (input or output), everything should be floating point.  Thoughts?

I'll commit something like that later, thanks for the review!

================
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);
+  });
+
----------------
chandlerc wrote:
> 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.
Oof, not sure what happened there, my brain was clearly off when I wrote the patch.

http://reviews.llvm.org/D8948

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