[PATCH] D111862: [AArch64] Canonicalize X*(Y+1) or X*(1-Y) to madd/msub

[weiwei via Phabricator via llvm-commits]

wwei added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13016
+      SDValue Opnd = Opc == ISD::ADD ? V->getOperand(1) : V->getOperand(0);
+      if (auto C = dyn_cast<ConstantSDNode>(Opnd)) {
+        return C->isOne();
----------------
sdesmalen wrote:
> nit: unnecessary curly braces.
Braces removed


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12896
+
+    if (Op0->getOpcode() == ISD::ADD &&
+        isa<ConstantSDNode>(Op0->getOperand(1))) {
----------------
sdesmalen wrote:
> wwei wrote:
> > sdesmalen wrote:
> > > I'd suggest collapsing all conditions together into a single lambda, so that it makes the code structurally simpler (less control flow) and more readable (you can directly see the exact pattern being matched). It may also help handle the case where the N0 is an ADD/SUB which does not match the conditions that follow, where N1 is just `Y + 1`. e.g. (X+Z)*(Y+1), isn't yet handled by your code.
> > > 
> > >   auto IsAddSubWith1= [](SDValue V) -> bool {
> > >     unsigned Opc = V->getOpcode();
> > >     if ((Opc == ISD::ADD || Opc == ISD::SUB) ) && V->hasOneUse()) {
> > >       SDValue Opnd = Opc == ISD::ADD ? V->getOperand(1) : V->getOperand(0);
> > >       if (auto C = dyn_cast<ConstantSDNode>(Opnd))
> > >         return C->isOne();
> > >     }
> > >     return false;
> > >   }
> > > 
> > >   if (IsAddSubWith1(N->getOperand(0)) {
> > >     // Rewrite
> > >   }
> > > 
> > >   if (IsAddSubWith1(N->getOperand(1))) {
> > >     // Rewrite
> > >   }
> > Thank you for your suggestion! I have refactored the code.
> If you change `IsAddSubWith1` also return the other operand, the code for rewriting becomes a bit simpler and easier to follow.
> 
> ```
> auto IsAddSubWith1 = [](SDValue V, SDValue &OtherOpnd) -> bool {
>   ...
> };
> 
> SDValue OtherOpnd;                                                
> if (IsAddSubWith1(N0, OtherOpnd)) {
>   SDValue MulVal = DAG.getNode(ISD::MUL, DL, VT, N1, OtherOpnd);
>   return DAG.getNode(N0->getOpcode(), DL, VT, N1, MulVal);
> }
> ```
Done.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111862/new/

https://reviews.llvm.org/D111862