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

[Zvi Rackover via llvm-commits]

zvi marked 6 inline comments as done.
zvi added inline comments.


================
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:
> zvi wrote:
> > 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.
> > 
> That's okay. Using isAnyConstantBuildVector is fine.
> My point was that the check for isUndef() in that function is not longer needed because you check in advance if N1 is undef.
> 
> 
You're right and I will improve this. 


Repository:
  rL LLVM

https://reviews.llvm.org/D25524