[PATCH] D124418: [CHR] Skip region containing llvm.coro.id

[Wei Wang via Phabricator via llvm-commits]

weiwang added a comment.

In D124418#3473558 <https://reviews.llvm.org/D124418#3473558>, @ChuanqiXu wrote:

> Hi, could you elaborate more what CHR does and why we need to skip coro.id? I don't know what happened now

Sure. We encountered this in one internal workload.

The ICE is trigged by an invalid cast at https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Coroutines/Coroutines.cpp#L231

  void coro::Shape::buildFrom(Function &F) {
    ... ...
    case Intrinsic::coro_begin: {
      auto Id = dyn_cast<CoroIdInst>(CB->getId());
    ... ...

The first argument of `llvm.coro.begin` intrinsic is expected to be `llvm.coro.id`, but it was actually a PHI node.

In fact, if IR verifier is turned on, the compilation would hit an assert after CHR pass (https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/Verifier.cpp#L3119)

A typical coroutine body has the following control flow

  entry:
    %id = call token @llvm.coro.id(...)
    %need.dyn.alloc = call i1 @llvm.coro.alloc(token %id)
    br i1 %need.dyn.alloc, label %coro.alloc, label %coro.init
  
  coro.alloc:
    %alloc = call i8* @custom_allocator(...)
    br label %coro.init
  
  coro.init:
    %phi = phi i8* [ null, %entry ], [ %alloc, %coro.alloc ]
    %hdl = call noalias nonnull i8* @llvm.coro.begin(token %id, i8* %phi)
  ... ...

This forms a region of 2 blocks `entry` and `coro.alloc` and with `coro.init` being the exit block.

It is possible that after `CoroSplit` pass, coroutine functions are inlined into other functions. And during subsequent `CHR` pass, when the blocks in such region are cloned, the control flow becomes

  ... ...
  entry:
    %id = call token @llvm.coro.id(...)
    %need.dyn.alloc = call i1 @llvm.coro.alloc(token %id)
    br i1 %need.dyn.alloc, label %coro.alloc, label %coro.init
  
  coro.alloc:
    %alloc = call i8* @custom_allocator(...)
    br label %coro.init
  
  ; clone of entry (cold path)
  entry.nonchr:
    %id.nonchr = call token @llvm.coro.id(...)
    %need.dyn.alloc.nonchr = call i1 @llvm.coro.alloc(token %id)
    br i1 %need.dyn.alloc.nonchr, label %coro.alloc.nonchr, label %coro.init
  
  ; clone of coro.alloc (cold path)
  coro.alloc.nonchr:
    %alloc.nonchr = call i8* @custom_allocator(...)
    br label %coro.init
  
  coro.init:
    %phi.id = phi token [%id, %entry], [%id, %cor.alloc], [%id.nonchr, %entry.nonchr], [%id.nonchr, %coro.alloc.nonchr]
    %phi.alloc = phi i8* [ null, %entry ], [ %alloc, %coro.alloc ], [ null, %entry.nonchr ], [ %alloc.nonchr, %coro.alloc.nonchr ]
    %hdl = call noalias nonnull i8* @llvm.coro.begin(token %phi.id, i8* %phi.alloc)
    ... ...

This creates an invalid PHI node with token type at the beginning of `coro.init` block.

The proposed fix checks for `llvm.coro.id` inside region, and if it is found, the region will be excluded from CHR. This way the `entry` block will not be cloned, and thus no PHI node will be created.

This could lead to less optimal codegen though, because the region is excluded, it can prevent CHR from merging adjacent regions into bigger scope and hoisting more branches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124418