[PATCH] D94230: [AArch64][SVE] Add SVE IR pass to coalesce ptrue instrinsic calls

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the update, the new structure looks good to me. I left a few more stylistic comments, but it would be good if someone more familiar with the details of the transform would take a look as well



================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:12
 //
-// The main goal of this pass is to remove unnecessary reinterpret
-// intrinsics (llvm.aarch64.sve.convert.[to|from].svbool), e.g:
+// The main goals of this pass are:
 //
----------------
nit: The wording seems a bit odd. Are there 'non-main' goals for the pass? Maybe just say something like `This pass performs the following optimizations: ....`


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:70
   static bool optimizeIntrinsic(Instruction *I);
+  bool optimizeIntrinsicCalls(SmallSetVector<Function *, 4> &Functions);
 
----------------
can you add comments explaining to scope `opimtizeIntrinsicCalls` and `optimizeFunctions` operate?


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:126
+  for (User *User : PTrue->users()) {
+    auto *IntrUser = dyn_cast<IntrinsicInst>(User);
+    if (IntrUser && IntrUser->getIntrinsicID() ==
----------------
nit: this could be something like
`    if (match(User, m_Intrinsic<Intrinsic::aarch64_sve_convert_to_svbool>(`, 

might save a line or two.



================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:263
+  for (auto *F : Functions) {
+    for (auto &BB : *F) {
+      SmallSetVector<IntrinsicInst *, 4> SVAllPTrues;
----------------
you just need to iterate over all instructions in a function, right? There's `instruction(F)` which does so directly.


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:273
+        auto *IntrI = dyn_cast<IntrinsicInst>(&I);
+        if (!(IntrI && IntrI->getIntrinsicID() == Intrinsic::aarch64_sve_ptrue))
+          continue;
----------------
nit: it would be simpler to read if the `!` would be moved inside?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94230