[PATCH] D84411: [LoopVersion] Pass RuntimePointerChecking directly.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/Analysis/LoopAccessAnalysis.h:342
   /// of success, false otherwise.
-  bool addPointer(unsigned Index);
+  bool addPointer(unsigned Index, RuntimePointerChecking &RtCheck);
 
----------------
Did you consider documenting the `RtCheck` parameter?


================
Comment at: llvm/include/llvm/Transforms/Utils/LoopUtils.h:444
 
 /// Add code that checks at runtime if the accessed arrays in \p PointerChecks
 /// overlap.
----------------
`\p PointerChecks`: needs to be updated


================
Comment at: llvm/include/llvm/Transforms/Utils/LoopVersioning.h:48
   /// object having no checks and we expect the user to add them.
-  LoopVersioning(const LoopAccessInfo &LAI,
-                 ArrayRef<RuntimePointerCheck> Checks, Loop *L, LoopInfo *LI,
+  LoopVersioning(const RuntimePointerChecking &RtPTrCheck,
+                 const SCEVUnionPredicate &Preds, Loop *L, LoopInfo *LI,
----------------
Did you consider documenting what the `RtPTrCheck` argument is used for?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:467
+    function_ref<bool(const RuntimePointerCheck &Check)> Predicate) {
+  Checks = {Checks.begin(), remove_if(Checks, Predicate)};
+}
----------------
Is this any different to `llvm::erase_if(Checks, Predicate)`? This also looks like a dangerous game of iterator invalidation by the assignment-operator.


================
Comment at: llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp:403
+
+    auto &Checks = RtPointerChecking.getChecks();
     LLVM_DEBUG(dbgs() << "\nPointer Checks (count: " << Checks.size()
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | Unnecessary auto ]]. At least I would not have expected it to be a SmallVectorImpl.


================
Comment at: llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp:525
+    RuntimePointerChecking RtPointerChecking = *LAI.getRuntimePointerChecking();
+    auto &Checks = filterMemchecks(Candidates, RtPointerChecking);
 
----------------
[style] Unnecessary auto. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84411