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

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13531
 
+static SDValue combineSVEMul(SDNode *N, unsigned Opc, SelectionDAG &DAG) {
+  SDLoc DL(N);
----------------
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


================
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) {
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96961