[PATCH] D118168: [LLVM] Introduce llvm.loop.finite metadata to represent loops which are known to iterate a finite number of times

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

This is close to an LGTM.  I was originally going to give a contingent LGTM, but then realized that prior comments have not yet been fully addressed.  If you refresh with the requested changes, I will LGTM.



================
Comment at: llvm/docs/LangRef.rst:6884
+of ``llvm.loop.mustprogress`` which simply says that the loop either may terminate,
+unwind or interact with the environment. All loops within a function marked with
+the attribute ``willreturn`` are finite by definition.
----------------
This last sentence is important, and I'd really like to see the dual stated explicitly as well.  Something along the lines of "A function which contains a collection of loops marked finite and no other cycles or function calls must be willreturn."

I really want the definition here tightly coupled to willreturn.  Allowing divergence in the definition seems like a really bad idea, and I want that said really explicitly in the LangRef.  


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:567
 
+void Loop::setLoopFinite() {
+  LLVMContext &Context = getHeader()->getContext();
----------------
This function appears to be unused in the patch.  Please removed.


================
Comment at: llvm/test/Analysis/ScalarEvolution/lt-overflow.ll:256
+
+define void @test_finite(i32 %S, i32 %N) {
+entry:
----------------
lebedev.ri wrote:
> Is this test missing the actual CHECK line?
This comment appears to have not been addressed.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118168