[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:12896
+
+    if (Op0->getOpcode() == ISD::ADD &&
+        isa<ConstantSDNode>(Op0->getOperand(1))) {
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12923
 
-  SDValue N0 = N->getOperand(0);
   ConstantSDNode *C = cast<ConstantSDNode>(N->getOperand(1));
   const APInt &ConstValue = C->getAPIntValue();
----------------
sdesmalen wrote:
> nit: this one can be replaced by N1.
done


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12930
      (N0->getOpcode() == ISD::TRUNCATE &&
       (IsSVECntIntrinsic(N0->getOperand(0)))))
        if (ConstValue.sge(1) && ConstValue.sle(16))
----------------
sdesmalen wrote:
> nit: this one can be replaced by N0.
This one can't be replaced


================
Comment at: llvm/test/CodeGen/AArch64/madd-combiner.ll:1-3
 ; RUN: llc -mtriple=aarch64-apple-darwin            -verify-machineinstrs < %s | FileCheck %s
 ; RUN: llc -mtriple=aarch64-apple-darwin -fast-isel -verify-machineinstrs < %s | FileCheck %s
+; RUN: llc -mtriple=aarch64-apple-darwin            -verify-machineinstrs < %s | FileCheck %s -check-prefixes=CHECK-MADD-MSUB
----------------
dmgreen wrote:
> Can you change these check lines to:
> ```
> ; RUN: llc -mtriple=aarch64-apple-darwin            -verify-machineinstrs < %s | FileCheck %s --check-prefixes=CHECK,CHECK-ISEL
> ; RUN: llc -mtriple=aarch64-apple-darwin -fast-isel -verify-machineinstrs < %s | FileCheck %s --check-prefixes=CHECK,CHECK-FAST
> ```
> And pre-generate the check lines with update_llc_test_checks.
updated, thanks


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

https://reviews.llvm.org/D111862