[PATCH] D45733: [DAGCombiner] Unfold scalar masked merge if profitable

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5381-5421
+  auto matchD = [](SDValue D, SDValue Y) -> llvm::Optional<SDValue> /*X*/ {
+    if (D.getOpcode() != ISD::XOR)
+      return llvm::None;
+    SDValue D0 = D->getOperand(0);
+    SDValue D1 = D->getOperand(1);
+    if (D1 == Y)
+      return D0;
----------------
spatel wrote:
> spatel wrote:
> > After stepping through more of your tests, I see why this is ugly.
> > 
> > We don't have to capture the intermediate values if the hasOneUse() checks are in the lambda(s) though. What do you think of this version:
> > 
> > ```
> >   // There are 3 commutable operators in the pattern, so we have to deal with
> >   // 8 possible variants of the basic pattern.
> >   SDValue X, Y, M;
> >   auto matchAndXor = [&X,&Y,&M](SDValue And, unsigned XorIdx, SDValue Other) {
> >     if (And.getOpcode() != ISD::AND || !And.hasOneUse())
> >       return false;
> >     if (And.getOperand(XorIdx).getOpcode() != ISD::XOR ||
> >         !And.getOperand(XorIdx).hasOneUse())
> >       return false;
> >     SDValue Xor0 = And.getOperand(XorIdx).getOperand(0);
> >     SDValue Xor1 = And.getOperand(XorIdx).getOperand(1);
> >     if (Other == Xor0) std::swap(Xor0, Xor1);
> >     if (Other != Xor1) return false;
> >     X = Xor0;
> >     Y = Xor1;
> >     M = And.getOperand(1);
> >     return true;
> >   };
> >   if (!matchAndXor(A, 0, B) && !matchAndXor(A, 1, B) &&
> >       !matchAndXor(B, 0, A) && !matchAndXor(B, 1, A))
> >     return SDValue();
> > 
> > ```
> Oops - forgot to swap the M capture:
>   M = And.getOperand(1);
> 
> should be:
>   M = And.getOperand(XorIdx ? 0 : 1);
> 
Well, the test changes are the same, and the code looks a bit shorter..
Though it is much harder to read.


Repository:
  rL LLVM

https://reviews.llvm.org/D45733