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

Sat Feb 22 08:10:21 PST 2020

jdoerfert added a comment.

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?

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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691