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

[Andrzej Warzynski via Phabricator via llvm-commits]

andwar added a comment.

Thanks for the updates @kmclaughlin ! Would you mind adding a comment to clearly mark the negative tests? E.g. for `@reinterpret_reductions_1`? Maybe also a comment `why` a particular case is not optimised? You've already done that for some tests, but not all of them.



================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:10
+//
+// Performs general IR level optimizations on SVE intrinsics.
+//
----------------
I don't see any documentation for the pass that's being added here (apart from the commit msg). Perhaps it's worth expanding this comment?


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:221
+bool SVEIntrinsicOpts::optimizeFunctions(
+    SmallVectorImpl<Function *> &Functions) {
+  bool Changed = false;
----------------
I think that you could simplify things a bit by using `SmallPtrSet` instead: http://llvm.org/docs/ProgrammersManual.html#dss-smallptrset.

With a set you can avoid explicit checks like this:
``` 
std::find(Functions.begin(), Functions.end(), Inst->getFunction()) == Functions.end()
```
With `SmallPtrSet` you can write less code :)


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:241
+
+  for (auto &F : M.getFunctionList()) {
+    if (!F.isDeclaration())
----------------
Could you decorate this `for` loop with a comment explaining `what` kind of data is generated here and `why`? Ta!


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

https://reviews.llvm.org/D76078