[PATCH] D25524: [DAGCombine] Preserve shuffles when one of the vector operands is constant
Andrea Di Biagio via llvm-commits
llvm-commits at lists.llvm.org
Sat Oct 22 01:40:29 PDT 2016
andreadb added a comment.
LGTM
Thanks Zvi!
================
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()))
----------------
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.
Repository:
rL LLVM
https://reviews.llvm.org/D25524
More information about the llvm-commits
mailing list