When a block containing llvm.coro.id is cloned during CHR, it inserts an invalid PHI node with token type to the beginning of the block containing llvm.coro.begin. To avoid such case, we exclude regions with llvm.coro.id.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
Hi, could you elaborate more what CHR does and why we need to skip coro.id? I don't know what happened now
Comment Actions
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.
Comment Actions
Thanks for the explanation. This fix LGTM.
| llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp | ||
|---|---|---|
| 776–778 | Maybe it is better to add a FIXME/NOTE to say we could hoist the coro.id intrinsic to the pos-dominator frontier. | |
Maybe it is better to add a FIXME/NOTE to say we could hoist the coro.id intrinsic to the pos-dominator frontier.