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

Thu Jan 27 12:12:36 PST 2022

jdoerfert added a comment.

In D118168#3277126 <https://reviews.llvm.org/D118168#3277126>, @wsmoses wrote:

> In D118168#3277051 <https://reviews.llvm.org/D118168#3277051>, @jdoerfert wrote:
>
>> I think this is generally sensible but lacks tests and a clear path towards usage. Who will produce the annotation?
>
> I think there are several target producers. For example, in here (https://github.com/llvm/llvm-project/issues/51103) an OpenMP for loop can produce it.

Can produce it, maybe yes. Now we should also have that setup or at least planned as we otherwise have no test coverage (esp. given the lack of tests).

================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:1345
+bool isFinite(const Loop *L);
+
 /// Return true if this loop can be assumed to make progress.  (i.e. can't
----------------
wsmoses wrote:
> jdoerfert wrote:
> > Why do we need both, esp. given the Note?
> For consistency with hasMustProgress? Happy to remove it if undesired.
Interestingly you split the two is/hasMP in the middle with this. On purpose?
The split for MP is an optimization of sorts, certainly not worth repeating if we are not using it right away.

================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:1126

+static const char *LLVMLoopFinite = "llvm.loop.finite";
+
----------------
wsmoses wrote:
> jdoerfert wrote:
> > While it seems to be the state-of-the-art to duplicate the string all over the place, could we please start a new scheme in which we have this definition early in the file and used whenever we need the string. Feel free to update the other ones as well.
> I concur, though I might prefer to split off the string restructuring from the langref change?
Works for me, string stuff I can review quickly

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118168