[PATCH] D74691: [Attributor] add some pattern to containsCycle

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D74691#1888074 <https://reviews.llvm.org/D74691#1888074>, @baziotis wrote:

> In D74691#1888062 <https://reviews.llvm.org/D74691#1888062>, @jdoerfert wrote:
>
> > In D74691#1887995 <https://reviews.llvm.org/D74691#1887995>, @baziotis wrote:
> >
> > > [...]
> > >  Btw, the search for some loops may need to happen in `AAUB`, since it is considered UB if I understood correctly.
> >
> >
> > So infinite loops without progress are UB, I think
>
>
> Me too, because of the C11 standard excerpt I quoted above.
>
> > but loops that might be infinite without progress are not necessarily. We can remove the latter and insert an unreachable for UB as always.
>
> Yes, that's what I meant. Assuming that here you meant "with", not "without" and "former", not "latter". Or otherwise I missed something.


Assuming forward-progress-guarantees, as we do in LLVM now, you can:

- Remove a loop that has no side-effect/sync, regardless of the trip count.
- Replace a loop that has no side-effect/sync with an unreachable, if it is proven endless.

The second transformation is "stronger" but also more restrictive. For example,

  while(c) {}

cannot be replaced with `unreachable` but can be removed if you don't know anything about `c`. You
can replace the loop with `llvm.assume(!c)` if you want.

>> The attribute would actually allow or prevent the above, deepening which default we choose. Basically opt-in or out of the forward progress which enables or disables the proposed reasoning.
> 
> Oh, ok. So, my assumption was:
> 
> - `forward-progress` would mean: You can remove side-effect-free infinite loops. One implication of that is: This function _definitely_ forwards progress although it might never return. This is also how I think people in llvm-dev wanted to use that: If you're compiling a C/C++ function, put this attribute so you can do this optimization. Otherwise, don't, so you can't.
> - On the other hand, `no-forward-progress-guarantee` I think does not make sense (because of its name) to mean: You can't remove such loops. I got that it means: "This function may get into a side-effect-free infinite loop". Which is a piece of information nonetheless.

`no-forward-progress-guarantee` would effectively mean that you cannot remove or replace loops that do not have side-effects/syncs. On way of thinking of this is: (non-)termination becomes observable and threads might be interrupted by events even if there is no direct control flow path.

> All in all, definitely I'm not the one to decide but I would be glad if you could clarify which of the above are planned (or you plan) to be added.

I'm not necessarily favoring a default but we need to propose this as an RFC on the list and see where people stand.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2383
+  for (scc_iterator<Function *> I = scc_begin(&F), IE = scc_end(&F); I != IE;
+       ++I) {
+    const std::vector<BasicBlock *> &SCCBBs = *I;
----------------
omarahmed wrote:
> jdoerfert wrote:
> > Nit: Maybe `for (auto &It : make_range(scc_begin(&F), scc_end(&F)))`
> > Or create a helper `scc_range(..)` that create the range.
> I am kind of stuck in using hasLoop function with using these method for looping :) , so how could I change It( vector<Block*>) to SCC_iter<Function *> type
Never mind then ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691