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

Stefanos Baziotis via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Feb 23 05:06:08 PST 2020


baziotis added a comment.

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

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


Ok, clearer, thanks!

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

Yes, it makes sense.

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

Me neither, and I'm positive to the RFC of course. If I can help in any way, ping me. I'll definitely take part in the review. :)



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2385
+
+    bool SCCHasLoop = It.hasLoop();
+    if (!SCCHasLoop)
----------------
There's https://reviews.llvm.org/D74801. If it gets committed before this, I'll notify you to just change the naming.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2394
+    Loop *L = LI->getLoopFor(SCCBBs.front());
+    // Check whether the current SCC is a loop or not.
+    if (!L)
----------------
To me, that's a little bit inaccurate. It seems like this `if` checks if the SCC is a loop or not. But that's actually the whole code that follows.
So, I'd replace it with a comment in the line above (exactly above the `getForLoop()`) since these 2 are coupled.
`If any random block in this SCC does not belong to a loop, then this SCC is definitely not a loop.`


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2398-2400
+    // Get the outer most loop and compare the number of blocks of the SCC and
+    // the loop to check if the current SCC is a loop cycle or not.If a loop
+    // then we check whether it is a bounded loop or unbounded loop.
----------------
One note is that we get the innermost loop with `getLoopFor()` rather than the outermost. Also, the part about the loop cycle was unclear to me. Still, I feel breaking it down into 3 cases is clearer but here's another take:
```
L is the innermost loop that has a common block with the SCC. Since a loop is always an SCC, if their number of blocks
are equal, the SCC is a loop - specifically, L. Otherwise, there are 2 cases:
- If the SCC has less blocks, then it is definitely not a loop.
- If it has more, then we can't decide since the SCC can be a parent loop of L. So, we perform the same test for
the parent of L.
```



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2414-2415
+
+    // Check if there is an outer SCC with no loop that is as big as the
+    // current SCC.
+    if (!L)
----------------
The SCC might have a loop inside, but it might not be a loop itself. I propose something like:
```
If L is nullptr, we found no loop that matches exactly the number of blocks of the SCC and so the SCC is not a loop
```
Feel free to revise it but still remain accurate. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74691





More information about the llvm-commits mailing list