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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits