[PATCH] D25524: [DAGCombine] Preserve shuffles when one of the vector operands is constant

[Zvi Rackover via llvm-commits]

zvi added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13737
 
+// Checks for a BUILD_VECTOR composed of either all-undef's, or constants
+// possibly mixed with undef's.
----------------
mkuper wrote:
> Any chance we can put this in a place that makes more sense?
> See discussion on D25685.
Sure


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13764
+  if (!N0->hasOneUse() || !N1->hasOneUse())
+    return {};
+  bool N0AnyConst = isAnyConstantBuildVector(N0.getNode()),
----------------
andreadb wrote:
> Anywhere else in this file we return an SDValue() when the combine is not successful. It would be nicer if you do the same in this function. So, I suggest that you replace all the occurrences of `return {}` with `return SDValue()`.
I must have done this subconsciously. Would never have done it in a sound mind :)


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13765-13773
+  bool N0AnyConst = isAnyConstantBuildVector(N0.getNode()),
+       N1AnyConst = isAnyConstantBuildVector(N1.getNode());
+  if (!(N0AnyConst && N1AnyConst) && N0.getOpcode() == ISD::BUILD_VECTOR &&
+      N1.getOpcode() == ISD::BUILD_VECTOR) {
+    if (N0AnyConst && !ISD::isBuildVectorAllZeros(N0.getNode()))
+      return {};
+    if (N1AnyConst && !ISD::isBuildVectorAllZeros(N1.getNode()))
----------------
andreadb wrote:
> If N1 is Undef, then we should never early exit. As I wrote in one of my previous comments, by construction N0 can't be Undef; only N1 can be Undef.
> 
> Therefore, you can skip the computation of N0AnyConst and N1AnyConst if N1 is known to be Undef.  What I am suggesting is to change that code into something like this:
> 
> ```
> if (!N1.isUndef()) {
>   bool N0AnyConst = isAnyConstantBuildVector(N0.getNode());
>   bool N1AnyConst = isAnyConstantBuildVector(N1.getNode());
>   if (N0AnyConst && !N1AnyConst && !ISD::isBuildVectorAllZeros(N0.getNode()))
>     return SDValue();
>   if (!N0AnyConst && N1AnyConst && !ISD::isBuildVectorAllZeros(N1.getNode()))
>     return SDValue();
> }
> ```
> 
> The extra check for `N->isUndef()` in `isAnyConstantBuildVector` would become redundant if your code checks in advance if N1 is Undef.
> 
I humbly accept your improvement.

>The extra check for N->isUndef() in isAnyConstantBuildVector would become redundant if your code checks in advance if N1 is Undef.

Not sure if you are suggesting to change

    bool N1AnyConst = isAnyConstantBuildVector(N1.getNode());

to

    bool N1AnyConst = ISD::isBuildVectorOfConstantSDNodes(N) ||
            ISD::isBuildVectorOfConstantFPSDNodes(N);

or that is just a side note.

I prefer the former for brevity.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13792-13793
+  }
+  if (Ops.size() != NumElts)
+    return {};
+  // BUILD_VECTOR requires all inputs to be of the same type, find the
----------------
andreadb wrote:
> This can never happen because we return at line 13787 if one of the operands cannot be combined.
> This if-stmt should be removed.
Will remove. Thanks.


Repository:
  rL LLVM

https://reviews.llvm.org/D25524