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

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm added a comment.

I'd like to see one-liner comments on each test explaining the spirit of what is being tested.



================
Comment at: llvm/lib/Target/AArch64/SVECoalescePTrues.cpp:14
+// Suppose that we have two SVE ptrue intrinsic calls P1 and P2. If P1 is at
+// encompasses P2, then P2 can be written as a reinterpret of P1 using the SVE
+// reinterpret intrinsics.
----------------
Non-grammatical text "If P1 is at encompasses P2". Also, definition of "encompasses" is missing here, I think this text should stand alone. I'd talk about supersets of active lane bits or similar instead.


================
Comment at: llvm/lib/Target/AArch64/SVECoalescePTrues.cpp:214
+  // Check for SVE intrinsic declarations first, and store the function where
+  // they are used so that we only iterate over relevant functions.
+  for (auto &F : M.getFunctionList()) {
----------------
If doing this, the comment should make clear: Why store upfront? Why not runOnFunction(f) for each function f?


================
Comment at: llvm/test/CodeGen/AArch64/sve-coalesce-ptrues.ll:5
+
+; If this check fails please read test/CodeGen/AArch64/README for instructions on how to resolve it.
+; WARN-NOT: warning
----------------
Just checking: is this comment applicable here? I'm thinking that since this fix doesn't relate to fixing any TypeSize warnings, it doesn't apply.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94230