[PATCH] D101302: [AArch64][SVE] Move convert.{from,to}.svbool optimization into InstCombine

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm requested changes to this revision.
peterwaller-arm added a comment.
This revision now requires changes to proceed.

Couple of things which look like they need tidying up.



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:289
+  SmallVector<Instruction *, 32> Worklist;
+  auto RequiredType = II.getType();
+
----------------
dmgreen wrote:
> auto *
> (And maybe just Type *)
Nit: Complaint from clang tidy.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:317
+    NPN->addIncoming(Reinterpret->getOperand(0), PN->getIncomingBlock(I));
+    Worklist.push_back(Reinterpret);
+  }
----------------
I see that the erasure of things from the worklist has been dropped, but not the worklist. Is it handled in replaceInstUsesWith?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:326
+                                                            IntrinsicInst &II) {
+  // If the reinterpret instruction operand is a PHI Node
+  if (isa<PHINode>(II.getArgOperand(0)))
----------------
Nit: Comment sounds incomplete and reads the same as the code. Suggestion: "Handle reinterprets of phi nodes." or drop the comment.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:357
+
+    CandidatesForRemoval.insert(CandidatesForRemoval.begin(), IntrinsicCursor);
+    Cursor = IntrinsicCursor->getOperand(0);
----------------
Another thing which appears not to have any uses.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101302