[PATCH] D98033: [AArch64][SVEIntrinsicOpts] Factor out redundant SVE mul/fmul intrinsics

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:377
+  auto *Op1 = I->getOperand(1);
+  auto *Op2 = I->getOperand(2);
+
----------------
Naming suggestions: OpPredicate, OpMultiplicand, OpMultiplier.


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:420
+  // [f]mul pg %n (dupx 1) => %n
+  else if (IsUnitDupX(Op2)) {
+    I->replaceAllUsesWith(Op1);
----------------
I've had a quick grep, and can't see much precedent for this style. If there are braces on the preceding if body, it seems to almost always appear as `} else if () {` (I couldn't find a counterexample). I can see why you've written it like this but the structure looks a bit too much like independent if's and I might have missed the else's.

Suggested syntax:

```
if () {
  // [f]mul pg (dupx 1) %n => %n
} else if () {
  // [f]mul pg %n (dupx 1) => %n
}
```

etc.


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:450
+    }
+  }
+
----------------
As discussed offline, you can check if the rightmost operand is what you're interested in, and if it's not, std::swap the two operands and recheck, to avoid duplication of the substitution logic across operands.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98033