[PATCH] D76078: [AArch64][SVE] Add a pass for SVE intrinsic optimisations

[Andrzej Warzynski via Phabricator via llvm-commits]

andwar added a comment.

Cheers for working on this @kmclaughlin ! Have you considered adding an interface for the new PM? You could check this for reference: https://reviews.llvm.org/rGd6de5f12d485a85504bc99d384a85634574a27e2 (also implements a FunctionPass).



================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:30
+
+#define DEBUG_TYPE "sve-intrinsicopts"
+
----------------
In AArch64TargetMachine.cpp it's:
```
static cl::opt<bool> EnableSVEIntrinsicOpts(
    "aarch64-sve-intrinsic-opts", cl::Hidden,
    cl::desc("Enable SVE intrinsic opts"),
    cl::init(true));
```
so it probably make sense to use `see-intrinsic-opts` here as well?


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:110
+  auto *PN = dyn_cast<PHINode>(X->getOperand(0));
+  if (!PN)
+    return false;
----------------
Given where this method is called, this is guaranteed to be always non-null. Perhaps `assert` instead?


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:120
+    if (!Reinterpret ||
+        RequiredType != Reinterpret->getArgOperand(0)->getType())
+      return false;
----------------
Isn't it guaranteed that `RequiredType == Reinterpret->getArgOperand(0)->getType()` is always true? I.e., `PN` and the incoming values have identical type.


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:125
+  // Create the new Phi
+  LLVMContext &C1 = PN->getContext();
+  IRBuilder<> Builder(C1);
----------------
[nit] Perhaps `Ctx` instead of `C1`?


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:131
+
+  for (unsigned i = 0; i < PN->getNumIncomingValues(); i++) {
+    auto *Reinterpret = cast<Instruction>(PN->getIncomingValue(i));
----------------
`i` -> `I`


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:187
+  // elide away both reinterprets if there are no other users of Y.
+  if (auto *Y = isReinterpretToSVBool(I->getArgOperand(0))) {
+    Value *SourceVal = Y->getArgOperand(0);
----------------
I'd be tempted to rewrite this to use early exit (https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code):

```
auto *Y = isReinterpretToSVBool(I->getArgOperand(0));
if (nullptr == Y)
  return false;

// The rest of the function
```


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:224
+  bool Changed = false;
+  for (auto II = BB->begin(), IE = BB->end(); II != IE;) {
+    Instruction *I = &(*II);
----------------
1. Could this be a for-range loop instead?

2. This loop seems to be a perfect candidate for `make_early_inc_range` (https://github.com/llvm/llvm-project/blob/172f1460ae05ab5c33c757142c8bdb10acfbdbe1/llvm/include/llvm/ADT/STLExtras.h#L499), e.g.
```
 for (Instruction &I : make_early_inc_range(BB))
  Changed |= optimizeIntrinsic(&I);
```


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:240
+  ReversePostOrderTraversal<BasicBlock *> RPOT(Root);
+  for (auto I = RPOT.begin(), E = RPOT.end(); I != E; ++I) {
+    Changed |= optimizeBlock(*I);
----------------
AFAIK, this iterates over BBs so `I` should be replaced with `BB`. 

Also, perhaps `for (auto *BB : RPOT) {` instead? 


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsic-opts-ptest.ll:18
+; OPT-LABEL: ptest_any2
+; OPT: %out = call i1 @llvm.aarch64.sve.ptest.any.nxv16i1(<vscale x 16 x i1> %1, <vscale x 16 x i1> %2)
+  %mask = tail call <vscale x 2 x i1> @llvm.aarch64.sve.ptrue.nxv2i1(i32 31)
----------------
What's `%1` and `%2`? Is it worth adding the calls that generated them in the expected output?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76078