[PATCH] D78460: [LAA] Move runtime-check generation to Transforms/Utils/loopUtils (NFC)

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

This analysis/utils refactoring looks good to me, thanks! Just adding some nits to clarify some presumably "add-on" parts, and possible alternatives/follow-ups.



================
Comment at: llvm/include/llvm/Transforms/Utils/LoopUtils.h:50
+    RuntimePointerCheck;
 
 template <typename T> class Optional;
----------------
nit: may be worthwhile to have a typdef for `SmallVectorImpl<RuntimePointerCheck>` as well.


================
Comment at: llvm/include/llvm/Transforms/Utils/LoopUtils.h:437
 
+/// Generate instructions for the run-time checks in \p PointerChecks.
+///
----------------
nit: would be good to retain some of the original explanation of the abandoned addRuntimeChecks():

```
/// Add code that checks at runtime if the accessed arrays overlap.
```


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1556
+  return nullptr;
+}
+
----------------
nit: is there a reason to swap the order of getFirstInst() and struct PointerBounds?


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1563
+                                  SCEVExpander &Exp, ScalarEvolution *SE) {
+  Value *Ptr = CG->RtCheck.Pointers[CG->Members[0]].PointerValue;
+  const SCEV *Sc = SE->getSCEV(Ptr);
----------------
Just to clarify: using CG->ReCheck to save passing PtrRtChecking parameter here and through the other expandBounds() could have been done as a preparatory patch, right?

Seems like CG should provide an API for retrieving the desired Ptr of its first member.


================
Comment at: llvm/lib/Transforms/Utils/LoopVersioning.cpp:65
   BasicBlock *RuntimeCheckBB = VersionedLoop->getLoopPreheader();
+  const auto &RtPtrChecking = *LAI.getRuntimePointerChecking();
   std::tie(FirstCheckInst, MemRuntimeCheck) =
----------------
Seems strange to call getRuntimePointerChecking() only for its getSE(). Perhaps pass RtPtrChecking, as commented below for LV, and have addRuntimeChecks() retrieve both AliasChecks and SE from there. This would require modifying the way (not) UseLAIChecks is handled.

Moreover, perhaps LoopVersioning's additional use of AliasChecks to annotate insns with no alias, should be shared with LV, or in general be taken care of by addRuntimeChecks()?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2795
+      addRuntimeChecks(MemCheckBlock->getTerminator(), OrigLoop,
+                       RtPtrChecking.getChecks(), RtPtrChecking.getSE());
+  assert(MemRuntimeCheck && "Expected runtime checks to be generated");
----------------
Just to clarify: inlining addRuntimeChecks(Insn)'s call to RtPtrChecking.Need and discarding the former could have been done as a preparatory patch, right?

OTOH, instead of passing both RtPtrChecking.getChecks() and RtPtrChecking.getSE(), and potentially also RtPtrChecking.Need, perhaps pass only RtPtrChecking. This early-exit check for Need could also apply to LoopVersioning, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78460