[PATCH] D96961: [AArch64][SVE][DAGCombine] Factor out redundant SVE mul/fmul intrinsics

Fri Feb 19 08:44:14 PST 2021

joechrisellis added inline comments.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13531

+static SDValue combineSVEMul(SDNode *N, unsigned Opc, SelectionDAG &DAG) {
+  SDLoc DL(N);
----------------
david-arm wrote:
> Is it worth naming this something like combineSVEIntrinsicBinOp and similarly for FP you could have combineSVEIntrinsicFPBinOp? A bit like SelectionDAG::simplifyFPBinop. The reason I mention this is that I can imagine you wanting similar things for divides, adds at some point too, i.e. fdiv X, 1.0 -> X or fadd X, 0.0 -> X
This is a great suggestion, thank you! Done.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13575
+  // If Pg is an all-true predicate...
+  if (getIntrinsicID(PgNode) == Intrinsic::aarch64_sve_ptrue &&
+      Pg.getConstantOperandVal(1) == AArch64SVEPredPattern::all) {
----------------
david-arm wrote:
> Perhaps you can commonise the check for "ptrue all" that appears in both functions? Also, it might be worth inverting the logic and moving to the start of the function, i.e.
> 
> ```
>   SDLoc DL(N);
> 
>   SDValue Pg = N->getOperand(1);
> 
>   if (!ptrueAllIntrinsic(Pg))
>     return SDValue();
> ```
> 
> This avoids the extra indentation. Not sure what you think?
Agreed -- done. 😄 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96961