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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

In D74691#1888062 <https://reviews.llvm.org/D74691#1888062>, @jdoerfert wrote:

> In D74691#1887995 <https://reviews.llvm.org/D74691#1887995>, @baziotis wrote:
>
> > In D74691#1887983 <https://reviews.llvm.org/D74691#1887983>, @jdoerfert wrote:
> >
> > > In D74691#1887970 <https://reviews.llvm.org/D74691#1887970>, @baziotis wrote:
> > >
> > > > In D74691#1887837 <https://reviews.llvm.org/D74691#1887837>, @jdoerfert wrote:
> > > >
> > > > > I added a bunch of comments. Mostly minor. I think this is almost done and I like how it turned out. Once the comments are addressed I'm fine with this, @baziotis feel free to accept then.
> > > >
> > > >
> > > > Ok, yes.
> > > >
> > > > > The next step to improve `willreturn` is actually to allow loops with unknown bounds, e.g., link-list traversals but also SCCs that are not loops. During initialize we collect all the ones that might be endless, and in the updateImpl we have to "justify" why they are not. The justification criterion is: they do not contain any synchronizing instruction. `AANoSync` can help with identifying those ;)
> > > > >  @omarahmed Let me know if that is something you want to look into (eventually). If so, we can talk about it more.
> > > > > 
> > > > > [EDIT: Now that I think about it for another second, we should probably split that logic I describe above into a new abstract attribute, e.g., `AALoopInformation`. We also need to sync up with @uenoku, I remember vaguely that he looked into something at similar at some point but it's all pretty foggy...]
> > > >
> > > > I may be missing something because I'm not familiar with synchronizing instructions but why does that guarantee that a loop will end? Or why can we assume it will end?
> > > >  I was reading the C11 standard:
> > > >
> > > >   An iteration statement whose controlling expression is not a constant expression, that performs no input/output operations, does not access volatile objects, and performs no synchronization or atomic operations in its body, controlling expression, or (in the case of a for statement) its expression-3, may be assumed by the implementation to terminate.
> > > >
> > > >
> > > > That is, basically we can assume a loop ends (even if it doesn't actually) if it is side-effect free. That is, it has no sync instructions, no read/write instructions and no volatile accesses.
> > > >  I don't know if we follow the C11 standard but don't we fill only 1 of 3 if we only test for synchronizing instructions?
> > >
> > >
> > > C/C++ atomics and volatile are considered synchronization, according to `AANoSync`, so we are good on that front I think. Or did you mean something else?
> >
> >
> > That's what I figured by reading `AANoSync` but I had some wrong terminology in my head for the term "atomic". So, yes, you're right.
> >
> > > The one thing we should do however is start a discussion about a `no-forward-process-guarantee` attribute on llvm-dev. People that do not come from C/C++ already asked for that (https://bugs.llvm.org/show_bug.cgi?id=965). If you read the bug comments and look at the dates, we have that problem for a while and I think the function attribute is the cleanest solution here.
> >
> > `no-forward-process-guarantee` is basically the attribute version of `containsPossiblyEndlessLoop()` right (when it returns true) ? (Although, I believe we should name this with `process` -> `progress`).
> >  I don't quite get why this will be helpful though. I got that Reid Kleckner proposed `forward-progress-guarantee` so that we can put into functions in which we can remove infinite loops with no side-effects.
> >
> > 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.

> 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.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691