Page MenuHomePhabricator

[coro async] Disable lifetime.start sinking for ABI::Async and ABI::Retcon
Needs ReviewPublic

Authored by aschwaighofer on Oct 1 2021, 11:34 AM.

Details

Reviewers
rjmccall
Summary

It does not handle loops correctly i.e it moves the lifetime.start
intrinsic into a loop rendering the stack object as not alive for part
of the loop.

entry:
  %obj = alloca i8
  lifetime.start(%obj)

  br loop

loop:
  coro.suspend()
  escape(%obj)
  cond_br %cond, label %exit, label loop

  br loop

exit:
  lifetime.end(%obj

After sinking:

entry:
  %obj = alloca i8
  br loop

loop:
  coro.suspend()
  lifetime.start(%obj)
  escape(%obj)
  cond_br %cond, label %exit, label loop

  br loop

exit:
  lifetime.end(%obj

rdar://83411917

Event Timeline

aschwaighofer created this revision.Oct 1 2021, 11:34 AM
aschwaighofer requested review of this revision.Oct 1 2021, 11:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2021, 11:34 AM

What's the justification for leaving this on for the switch lowering? It sounds like it's a broken optimization that could trigger in any sort of coroutine.

@rjmccall I am not familiar with (the possible constraints of/assumptions) switch lowering but I would assume that it is broken there too (I don't have the time to investigate so I did not want to regress them needlessly).

aschwaighofer added a comment.EditedOct 6 2021, 11:16 AM

@lxfind What do you think, is that scenario also possible with Switch lowering. Should we disable lifetime sinking and lifetime based optimization (https://reviews.llvm.org/D110949) for switch lowering until it is fixed?

An example like the one mentioned in the commit message will fail after we have sunk the lifetime.start into the loop because the check whether we cross a suspend by starting at the lifetime.begin will fail leading us to use a localized alloca rather than a frame entry.

Another way of thinking about it is that by sinking the lifetime.start we are saying that the memory location is unitialized between the lifetime.start and the first write to its location (%obj). That is definitely not true along the path through the back-edge (other optimizations could mis-optimize based on this).

entry:
  %obj = alloca i8
  lifetime.start(%obj) // <== sink from here

  br loop

loop:
  coro.suspend()
  lifetime.start(%obj)  // <== to here
  escape(%obj)
  cond_br %cond, label %exit, label loop

exit:
  lifetime.end(%obj)

In terms of IR constrains it seems we are violating this constraint (https://llvm.org/docs/LangRef.html#llvm-lifetime-start-intrinsic) by performing the sinking in the example listed above:

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.