Skip to content

[Coroutines] Drop dead instructions more aggressively in addMustTailToCoroResumes() #85271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Mar 20, 2024
Merged

[Coroutines] Drop dead instructions more aggressively in addMustTailToCoroResumes() #85271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
97c0ff6
[Coroutines] Drop dead instructions more aggressively in addMustTailT…
zmodem Mar 14, 2024
e268280
skip any side-effect free instructions when looking for the return
zmodem Mar 15, 2024
bbf499c
return %foo in the lit test
zmodem Mar 20, 2024
98f917b
add clang codegen test
zmodem Mar 20, 2024
9b30f17
use wouldInstructionBeTriviallyDead() instead
zmodem Mar 20, 2024
e1ec1a5
add link in coro-symmetric-transfer-04.cpp
zmodem Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions clang/test/CodeGenCoroutines/coro-symmetric-transfer-04.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// This tests that the symmetric transfer at the final suspend point could happen successfully.
// Based on https://github.com/llvm/llvm-project/pull/85271#issuecomment-2007554532
// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 -O2 -emit-llvm %s -o - | FileCheck %s

#include "Inputs/coroutine.h"

struct Task {
struct promise_type {
struct FinalAwaiter {
bool await_ready() const noexcept { return false; }
template <typename PromiseType>
std::coroutine_handle<> await_suspend(std::coroutine_handle<PromiseType> h) noexcept {
return h.promise().continuation;
}
void await_resume() noexcept {}
};
Task get_return_object() noexcept {
return std::coroutine_handle<promise_type>::from_promise(*this);
}
std::suspend_always initial_suspend() noexcept { return {}; }
FinalAwaiter final_suspend() noexcept { return {}; }
void unhandled_exception() noexcept {}
void return_value(int x) noexcept {
_value = x;
}
std::coroutine_handle<> continuation;
int _value;
};

Task(std::coroutine_handle<promise_type> handle) : handle(handle), stuff(123) {}

struct Awaiter {
std::coroutine_handle<promise_type> handle;
Awaiter(std::coroutine_handle<promise_type> handle) : handle(handle) {}
bool await_ready() const noexcept { return false; }
std::coroutine_handle<void> await_suspend(std::coroutine_handle<void> continuation) noexcept {
handle.promise().continuation = continuation;
return handle;
}
int await_resume() noexcept {
int ret = handle.promise()._value;
handle.destroy();
return ret;
}
};

auto operator co_await() {
auto handle_ = handle;
handle = nullptr;
return Awaiter(handle_);
}

private:
std::coroutine_handle<promise_type> handle;
int stuff;
};

Task task0() {
co_return 43;
}

// CHECK-LABEL: define{{.*}} void @_Z5task0v.resume
// This checks we are still in the scope of the current function.
// CHECK-NOT: {{^}}}
// CHECK: musttail call fastcc void
// CHECK-NEXT: ret void
79 changes: 29 additions & 50 deletions llvm/lib/Transforms/Coroutines/CoroSplit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1197,22 +1197,6 @@ static bool simplifyTerminatorLeadingToRet(Instruction *InitialInst) {
assert(InitialInst->getModule());
const DataLayout &DL = InitialInst->getModule()->getDataLayout();

auto GetFirstValidInstruction = [](Instruction *I) {
while (I) {
// BitCastInst wouldn't generate actual code so that we could skip it.
if (isa<BitCastInst>(I) || I->isDebugOrPseudoInst() ||
I->isLifetimeStartOrEnd())
I = I->getNextNode();
else if (isInstructionTriviallyDead(I))
// Duing we are in the middle of the transformation, we need to erase
// the dead instruction manually.
I = &*I->eraseFromParent();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use RecursivelyDeleteTriviallyDeadInstructions() here instead? simplify seems overkill.

also do you know why we would end up with dead instructions at this point? we should have already simplified the function when we get to corosplit

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use RecursivelyDeleteTriviallyDeadInstructions() here instead? simplify seems overkill.

In my test case the first instruction isn't trivially dead, so I don't think that would work.

do you know why we would end up with dead instructions at this point? we should have already simplified the function when we get to corosplit

I think such instructions can emerge during corosplit. Maybe @ChuanqiXu9 can answer that one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use RecursivelyDeleteTriviallyDeadInstructions() here instead? simplify seems overkill.

In my test case the first instruction isn't trivially dead, so I don't think that would work.

if we call it on the second instruction, it'll delete the first one as well

do you know why we would end up with dead instructions at this point? we should have already simplified the function when we get to corosplit

I think such instructions can emerge during corosplit. Maybe @ChuanqiXu9 can answer that one.

understanding this a bit better would be good to see if we're doing extra unnecessary work, or if those instructions should be cleaned up earlier in the pass

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, since these instructions become dead during CoroSplit pass. The internals are

  %save2 = call token @llvm.coro.save(ptr null)
  call fastcc void @fakeresume1(ptr align 8 null)
  %suspend2 = call i8 @llvm.coro.suspend(token %save2, i1 false)

  ; These (non-trivially) dead instructions are in the way.
  %gep = getelementptr inbounds i64, ptr %alloc.var, i32 0
  %foo = ptrtoint ptr %gep to i64

  switch i8 %suspend2, label %exit [
    i8 0, label %await.ready
    i8 1, label %exit
  ]
await.ready:
  call void @consume(ptr %alloc.var)
  call void @llvm.lifetime.end.p0(i64 1, ptr %alloc.var)
  br label %exit
exit:
  call i1 @llvm.coro.end(ptr null, i1 false, token none)
  ret void
}

During the splitting, the value %suspend2 will become a constant in different splitted functions. So it makes sense if the previous passes can't catch such things.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

understanding this a bit better would be good to see if we're doing extra unnecessary work, or if those instructions should be cleaned up earlier in the pass

+1 I'd like to understand this better too :)

In the original code I was debugging, the presplit coroutine has a return value:

169:                                              ; preds = %166, %163, %135, %68, %48
  %170 = getelementptr inbounds i8, ptr %17, i64 -16
  %171 = ptrtoint ptr %170 to i64
  %172 = call i1 @llvm.coro.end(ptr null, i1 false) #19                                                                                 
  ret i64 %171                     
}

corosplit clones the function into various versions, and the modifications of those clones are what causes dead code, makes some values constant, etc.

In my case, CoroCloner::create() will make the original return unreachable:

  switch (Shape.ABI) {                                                  
  // In these ABIs, the cloned functions always return 'void', and the
  // existing return sites are meaningless.  Note that for unique 
  // continuations, this includes the returns associated with suspends;
  // this is fine because we can't suspend twice.
  case coro::ABI::Switch:                                                
  case coro::ABI::RetconOnce:
    // Remove old returns.
    for (ReturnInst *Return : Returns)
      changeToUnreachable(Return);                                     
    break;

and replaceCoroEnds() will replace the @llvm.coro.end by ret void.

So perhaps CoroCloner is the one who should clean up these instructions.

On the other hand, it seems corosplit relies on later passes to clean up this stuff, which is maybe fine (especially since it also runs at -O0), except that it's a problem for addMustTailToCoroResumes().

simplify seems overkill

Yes, maybe it's too big a hammer. On the other hand, the ad-hoc dce and constant folding in simplifyTerminatorLeadingToRet() makes me a little nervous.

Instead of deleting those dead instructions, maybe we could just look past them. What the code is really trying to do is follow the control flow to a return instruction. I think it should be able to skip any side-effect free instructions on the way as long as the values aren't needed for a conditional branch or such. I'll push a version which does that. Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So perhaps CoroCloner is the one who should clean up these instructions.

Generally this is not something we'd like to do for the middle end passes. However, this is (was) a defect in the clang/llvm coroutines implementations. Since the symmetric transfer is semantics about C++ and according to our design for modules, we have to make it in the middle end. So this is the why we have this... But probably we can never get rid of this if we (Clang/LLVM) don't want to be conforming to C++ anymore.

Yes, maybe it's too big a hammer. On the other hand, the ad-hoc dce and constant folding in simplifyTerminatorLeadingToRet() makes me a little nervous.

So my line is, if the problem comes from a particular C++ programs and the corresponding LLVM passes applied, yes we need to solve it. However, if it comes from a manual constructed IR only, then possibly we shouldn't do that.

else
break;
}
return I;
};

auto TryResolveConstant = [&ResolvedValues](Value *V) {
auto It = ResolvedValues.find(V);
if (It != ResolvedValues.end())
Expand All @@ -1221,8 +1205,9 @@ static bool simplifyTerminatorLeadingToRet(Instruction *InitialInst) {
};

Instruction *I = InitialInst;
while (I->isTerminator() || isa<CmpInst>(I)) {
while (true) {
if (isa<ReturnInst>(I)) {
assert(!cast<ReturnInst>(I)->getReturnValue());
ReplaceInstWithInst(InitialInst, I->clone());
return true;
}
Expand All @@ -1246,54 +1231,48 @@ static bool simplifyTerminatorLeadingToRet(Instruction *InitialInst) {

BasicBlock *Succ = BR->getSuccessor(SuccIndex);
scanPHIsAndUpdateValueMap(I, Succ, ResolvedValues);
I = GetFirstValidInstruction(Succ->getFirstNonPHIOrDbgOrLifetime());

I = Succ->getFirstNonPHIOrDbgOrLifetime();
continue;
}

if (auto *CondCmp = dyn_cast<CmpInst>(I)) {
if (auto *Cmp = dyn_cast<CmpInst>(I)) {
// If the case number of suspended switch instruction is reduced to
// 1, then it is simplified to CmpInst in llvm::ConstantFoldTerminator.
auto *BR = dyn_cast<BranchInst>(
GetFirstValidInstruction(CondCmp->getNextNode()));
if (!BR || !BR->isConditional() || CondCmp != BR->getCondition())
return false;

// And the comparsion looks like : %cond = icmp eq i8 %V, constant.
// So we try to resolve constant for the first operand only since the
// second operand should be literal constant by design.
ConstantInt *Cond0 = TryResolveConstant(CondCmp->getOperand(0));
auto *Cond1 = dyn_cast<ConstantInt>(CondCmp->getOperand(1));
if (!Cond0 || !Cond1)
return false;

// Both operands of the CmpInst are Constant. So that we could evaluate
// it immediately to get the destination.
auto *ConstResult =
dyn_cast_or_null<ConstantInt>(ConstantFoldCompareInstOperands(
CondCmp->getPredicate(), Cond0, Cond1, DL));
if (!ConstResult)
return false;

ResolvedValues[BR->getCondition()] = ConstResult;

// Handle this branch in next iteration.
I = BR;
continue;
// Try to constant fold it.
ConstantInt *Cond0 = TryResolveConstant(Cmp->getOperand(0));
ConstantInt *Cond1 = TryResolveConstant(Cmp->getOperand(1));
if (Cond0 && Cond1) {
ConstantInt *Result =
dyn_cast_or_null<ConstantInt>(ConstantFoldCompareInstOperands(
Cmp->getPredicate(), Cond0, Cond1, DL));
if (Result) {
ResolvedValues[Cmp] = Result;
I = I->getNextNode();
continue;
}
}
}

if (auto *SI = dyn_cast<SwitchInst>(I)) {
ConstantInt *Cond = TryResolveConstant(SI->getCondition());
if (!Cond)
return false;

BasicBlock *BB = SI->findCaseValue(Cond)->getCaseSuccessor();
scanPHIsAndUpdateValueMap(I, BB, ResolvedValues);
I = GetFirstValidInstruction(BB->getFirstNonPHIOrDbgOrLifetime());
BasicBlock *Succ = SI->findCaseValue(Cond)->getCaseSuccessor();
scanPHIsAndUpdateValueMap(I, Succ, ResolvedValues);
I = Succ->getFirstNonPHIOrDbgOrLifetime();
continue;
}

if (I->isDebugOrPseudoInst() || I->isLifetimeStartOrEnd() ||
wouldInstructionBeTriviallyDead(I)) {
// We can skip instructions without side effects. If their values are
// needed, we'll notice later, e.g. when hitting a conditional branch.
I = I->getNextNode();
continue;
}

return false;
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, we shouldn't do this. This is the pattern we generally do for optimizations. But here, it is not an pure optimizations. We are implementing a C++ semantics. (See my above comments for some backgrounds.) So it is better to break early if we have something we are not able to handle.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I took another look on this to make sure that I understood this patch correctly. And it looks like my previous comments were confusing and maybe unrelated. So ignore the above comments if it is confusing.

And the comment about wanting a new test from C++ still works.

Since the dirty properties of the corosplit pass mentioned above, it is hard to tell if this patch is correct or not easily. It depends on how the code will be generated from C++ frontend and other passes. So conservatively, this change doesn't cover the original codes since the trivally dead instructions may have side effects. So the conservative change is to make this patch covers previous behavior.

Otherwise, if we want to proceed aggressively, we may need to understand the original motivation issue and the corresponding C++ codes and invest if this is regression or new code patterns we never thought before. But as I said, it may be annoying since the root cause is dirty.

I can accept either the conservative or the aggressive method to proceed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. I've added a C++ reproducer.

If an instruction has side effects, it will not be considered trivially dead, so I think my patch covers the original cases, and more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an instruction has side effects, it will not be considered trivially dead, so I think my patch covers the original cases, and more.

It is true abstractly. But for LLVM, at least from the comment, it says:

/// Return true if the result produced by the instruction is not used, and the
/// instruction will return. Certain side-effecting instructions are also
/// considered dead if there are no uses of the instruction.
bool isInstructionTriviallyDead(Instruction *I,
                                const TargetLibraryInfo *TLI = nullptr);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll change it to call wouldInstructionBeTriviallyDead() instead.

}

return false;
Expand Down
12 changes: 9 additions & 3 deletions llvm/test/Transforms/Coroutines/coro-split-musttail7.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
; Tests that sinked lifetime markers wouldn't provent optimization
; to convert a resuming call to a musttail call.
; The difference between this and coro-split-musttail5.ll and coro-split-musttail5.ll
; The difference between this and coro-split-musttail5.ll and coro-split-musttail6.ll
; is that this contains dead instruction generated during the transformation,
; which makes the optimization harder.
; RUN: opt < %s -passes='cgscc(coro-split),simplifycfg,early-cse' -S | FileCheck %s
; RUN: opt < %s -passes='pgo-instr-gen,cgscc(coro-split),simplifycfg,early-cse' -S | FileCheck %s

declare void @fakeresume1(ptr align 8)

define void @g() #0 {
define i64 @g() #0 {
entry:
%id = call token @llvm.coro.id(i32 0, ptr null, ptr null, ptr null)
%alloc = call ptr @malloc(i64 16) #3
Expand All @@ -27,6 +27,11 @@ await.suspend:
%save2 = call token @llvm.coro.save(ptr null)
call fastcc void @fakeresume1(ptr align 8 null)
%suspend2 = call i8 @llvm.coro.suspend(token %save2, i1 false)

; These (non-trivially) dead instructions are in the way.
%gep = getelementptr inbounds i64, ptr %alloc.var, i32 0
%foo = ptrtoint ptr %gep to i64

switch i8 %suspend2, label %exit [
i8 0, label %await.ready
i8 1, label %exit
Expand All @@ -36,8 +41,9 @@ await.ready:
call void @llvm.lifetime.end.p0(i64 1, ptr %alloc.var)
br label %exit
exit:
%result = phi i64 [0, %entry], [0, %entry], [%foo, %await.suspend], [%foo, %await.suspend], [%foo, %await.ready]
call i1 @llvm.coro.end(ptr null, i1 false, token none)
ret void
ret i64 %result
}

; Verify that in the resume part resume call is marked with musttail.
Expand Down