[PATCH] D140231: CoroFrame: Put escaped variables with multiple lifetimes on coroutine frame

Mon Dec 19 00:15:59 PST 2022

MatzeB added a comment.

In D140231#4004061 <https://reviews.llvm.org/D140231#4004061>, @ChuanqiXu wrote:

> The patch is self-contained and good.
>
> I am curious about why https://github.com/llvm/llvm-project/issues/51843 is arm related since the patch doesn't involve the backend.

The issues is somewhat accidentally hitting ARM, because for the program in question passing a struct with 2 members, is modeled as 2xi64 vector type by the arm calling convention. This means there are instructions constructing this 2xi64 vector and those happen to get hoisted out of a loop. 1 of those 2 values is the address of a local variable (casted to i64 because of the calling convention) so the address suddenly needs to be constant even across the resume point within the loop and even though the variable is not alive throughout the whole loop as marked by the lifetime.start/end intrinsics.
The issue does not manifest on x86_64 because the struct calling convention uses 2 (separate) integer values instead of constructing a vector and those are simple enough to not get hoisted out of the loop it seems).

> And another point is about the assumption:
>
>> The llvm.lifetime.start intrinsic guarantees that the address for a given alloca is always the same.

I expressed this a bit sloppily. It's always the same for a single call (or for a coroutine a single call/ramp-up and all following suspendends and continuations. It obviously doesn't need to be the same for separate calls.

> In the manual (https://llvm.org/docs/LangRef.html#int-lifestart), it says:
>
>> After llvm.lifetime.end is called, ‘llvm.lifetime.start’ on the stack object can be called again. The second ‘llvm.lifetime.start’ call marks the object as alive, but it does not change the address of the object.
>
> I want to say these two statements doesn't look the same. Maybe it'll be better to send another patch to edit the manual too?

The manual is fine as-is and doesn't need a change, doesn't it?

PS: I am hearing reports that my fix repairs some of the problems our users had but not all of them, so I will do some more testing and investigation before landing this.

================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:82-83
 //   Kills: a bit vector which contains a set of indices of blocks that can
-//          reach block 'i', but one of the path will cross a suspend point
+//          reach block 'i' but there is a path crossing a suspend point
+//          not repeating 'i' (path to 'i' without cycles containing 'i').
 //   Suspend: a boolean indicating whether block 'i' contains a suspend point.
----------------
ChuanqiXu wrote:
> Is this change necessary? I mean if there is a path that can reach block 'i' and repeating 'i' , we must can reduce a path which can reach 'I' without repeating 'I'. Do I misunderstand anything?
I am clarifying the definition for a case like the following and a query whether "B kills B":

```
A
↓
B           ← ←
↓             ↑
C (suspend) → ↑
↓
D
```

If you queried whether the path from "B" to "B" crosses a suspend point, the "Kill" flag flags compute here reported `false`. This result does ignore the path around the loop B->C->B though, which is another way to reach B and contains a suspense point in C.

For most users in this file this interpretation is fine as it compares a "Def" and a "Use" and both being in the same block, just means the Def and the Use is in the same block and it doesn't matter that there we could also reach the block with a loop. (...And we may or not be right in this assumption that the "Def" always comes before the "Use" in the same block; but that is a discussion of corner cases for another day...)

In the case of this change however when comparing lifetime.start values however we do need to test whether any path from a lifetime.start to a lifetime.start contains a suspend point even if it is the same point reached through a loop. Hence I introduce the new `KillLoop` flag here which captures this loop situation as the "Kills" flag alone does not catch it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140231