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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/LoopUtils.h:437
 
+/// Generate instructions for the run-time checks in \p PointerChecks.
+///
----------------
Ayal wrote:
> 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.
> ```
I've reworded the first line with the more descriptive message, thanks!


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1556
+  return nullptr;
+}
+
----------------
Ayal wrote:
> nit: is there a reason to swap the order of getFirstInst() and struct PointerBounds?
I've moved it into a lambda in the single function using it.


================
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);
----------------
Ayal wrote:
> 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.
> 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?

I've pushed 57fb56b30e85c8e9662075c671d02fbdc37d8f3b doing exactly that, thanks.

> Seems like CG should provide an API for retrieving the desired Ptr of its first member.
Can do that as a follow up.


================
Comment at: llvm/lib/Transforms/Utils/LoopVersioning.cpp:65
   BasicBlock *RuntimeCheckBB = VersionedLoop->getLoopPreheader();
+  const auto &RtPtrChecking = *LAI.getRuntimePointerChecking();
   std::tie(FirstCheckInst, MemRuntimeCheck) =
----------------
Ayal wrote:
> 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()?
> 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.

I tried that, but it seems the LoopVersioning stores the checks on purpose. When using getChecks() directly, there are some test failures, although I did not spot the places that changes the checks on a quick look. I think unifying them would be better as a follow-up patch, if possible.

> 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()?

+1. Not sure if there is a reason for the split, but I can take a look as a follow-up/


================
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");
----------------
Ayal wrote:
> 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?
> Just to clarify: inlining addRuntimeChecks(Insn)'s call to RtPtrChecking.Need and discarding the former could have been done as a preparatory patch, right?

Done, I've put up D79679

> 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?
It looks like there's an issue with passing RtPtrchecking directly for LoopVersioning. I can look into what is going on there, but I think it would be better as follow-up, because it seems a bit more risky.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78460