[PATCH] D51553: [DAGCombiner][x86] add transform/hook to load a scalar directly for use in a vector binop

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

This looks reasonable to me, some nits.



================
Comment at: include/llvm/CodeGen/TargetLowering.h:535
 
+  virtual bool shouldLoadScalarIntoVectorOp(SDValue InsElt, SDValue Op) const {
+    return false;
----------------
You probably want to add some explanatory comment here.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15106
+  //       other constant elements as needed.
+  return Op0IsLoad ? DAG.getNode(BO.getOpcode(), DL, VecVT, NewInsert, C)
+                   : DAG.getNode(BO.getOpcode(), DL, VecVT, C, NewInsert);
----------------
Do we need to check that `BO.getOpcode()` is actually valid for `VecVT`?
Or are we ok with scalarization?


================
Comment at: test/CodeGen/X86/load-scalar-as-vector.ll:23
+; AVX2-NEXT:    vmovd {{.*#+}} xmm0 = mem[0],zero,zero,zero
+; AVX2-NEXT:    vpbroadcastd {{.*#+}} xmm1 = [42,42,42,42]
+; AVX2-NEXT:    vpaddd %xmm1, %xmm0, %xmm0
----------------
Why is there a `vpbroadcastd` here, as compared to `AVX1` version?


https://reviews.llvm.org/D51553