[Coroutines][LazyCallGraph] resumes are not really SCC #116285
Conversation
In this debug code the check assumes the lookup find an object. If it
does not it tries to dereference the nullptr. This patch adds a check,
however, notice it does not require the object to exist.
This method (addSplitRefRecursiveFunctions) was added for coroutines
with continuations. The split coroutine transform generates the
continuations by cloning the entire function then changing the entry
block and a few other things. This results in a lot of dead code that is
kept around only to satisfy a few assumptions then is removed.
With the deadcode the original function and its continuations appear to
be SCC. However, when the deadcode is removed they are not SCC. Consider
a simple coroutine { begin ... suspend ... suspend ... end }. After
deadcode is eliminated the ramp references resume0, resume0 references
resume1, etc...
To be efficient it is necessary to split coroutines without generating
deadcode and this removese the unnecessary references (on a phi's edges).
This does not satisfy one of the conditions of addSplitRefRecursiveFunctions:
"All new functions must reference (not call) each other.".
From my testing so far it seems that this condition is not necessary and
it is only this debug code that checks for it.
Can we safely remove the "All new functions must reference (not call) each
other." requirement of addSplitRefRecursiveFunctions?
If not, what alternative do we have so we avoid cloning deadcode? (In
our use case the deadcode can be a particular problem due to its size).
|
@llvm/pr-subscribers-coroutines Author: Tyler Nowicki (TylerNowicki) ChangesIn this debug code the check assumes the lookup find an object. If it does not it tries to dereference the nullptr. This patch adds a check, however, notice it does not require the object to exist. This method (addSplitRefRecursiveFunctions) was added for coroutines with continuations. The split coroutine transform generates the continuations by cloning the entire function then changing the entry block and a few other things. This results in a lot of dead code that is kept around only to satisfy a few assumptions then is removed. With the dead code the original function and its continuations appear to be SCC (all functions reference all other functions). However, when the dead code is removed they are not SCC. Consider a simple coroutine { begin ... suspend ... suspend ... end }. After dead code is eliminated the ramp only references resume0, resume0 only references resume1, etc... I found the original PR [1] for addSplitRefRecursiveFunctions, but it seems to assume that the new resume functions will be a SCC. In our use case the dead code can be a particular problem due to its size. For compile-time/size it is necessary to split the coroutine without generating dead code (e.g. CloneAndPrune). However, this also removes the dead references (on a phi's edges) to the other resume functions. The resulting resume functions are not an SCC, but this contradicts what has been written about addSplitRefRecursiveFunctions "To keep things simple, this only supports the case where all new edges are ref edges, and every new function references every other new function." [1]. The comments on are a little more relaxed saying "All new functions must reference (not call) each other." Does this mean that each resume function doesn't need to reference every other resume function? From my testing it seems that this condition can be relaxed, and it is only this debug code that checks that "every other new function" is referenced! Can we safely relax this such that the set of new (resume) functions is not necessarily an SCC? [1] https://reviews.llvm.org/D93828#change-Xmlqywc9evLH Full diff: https://github.com/llvm/llvm-project/pull/116285.diff 1 Files Affected:
diff --git a/llvm/lib/Analysis/LazyCallGraph.cpp b/llvm/lib/Analysis/LazyCallGraph.cpp
index 5aa36bfc36d468..724bd0ff416c8a 100644
--- a/llvm/lib/Analysis/LazyCallGraph.cpp
+++ b/llvm/lib/Analysis/LazyCallGraph.cpp
@@ -1775,8 +1775,9 @@ void LazyCallGraph::addSplitRefRecursiveFunctions(
if (F1 == F2)
continue;
Node &N2 = get(*F2);
- assert(!N1->lookup(N2)->isCall() &&
- "Edges between new functions must be ref edges");
+ assert(!N1->lookup(N2) ||
+ (!N1->lookup(N2)->isCall() &&
+ "Edges between new functions must be ref edges"));
}
}
#endif
|
|
@llvm/pr-subscribers-llvm-analysis Author: Tyler Nowicki (TylerNowicki) ChangesIn this debug code the check assumes the lookup find an object. If it does not it tries to dereference the nullptr. This patch adds a check, however, notice it does not require the object to exist. This method (addSplitRefRecursiveFunctions) was added for coroutines with continuations. The split coroutine transform generates the continuations by cloning the entire function then changing the entry block and a few other things. This results in a lot of dead code that is kept around only to satisfy a few assumptions then is removed. With the dead code the original function and its continuations appear to be SCC (all functions reference all other functions). However, when the dead code is removed they are not SCC. Consider a simple coroutine { begin ... suspend ... suspend ... end }. After dead code is eliminated the ramp only references resume0, resume0 only references resume1, etc... I found the original PR [1] for addSplitRefRecursiveFunctions, but it seems to assume that the new resume functions will be a SCC. In our use case the dead code can be a particular problem due to its size. For compile-time/size it is necessary to split the coroutine without generating dead code (e.g. CloneAndPrune). However, this also removes the dead references (on a phi's edges) to the other resume functions. The resulting resume functions are not an SCC, but this contradicts what has been written about addSplitRefRecursiveFunctions "To keep things simple, this only supports the case where all new edges are ref edges, and every new function references every other new function." [1]. The comments on are a little more relaxed saying "All new functions must reference (not call) each other." Does this mean that each resume function doesn't need to reference every other resume function? From my testing it seems that this condition can be relaxed, and it is only this debug code that checks that "every other new function" is referenced! Can we safely relax this such that the set of new (resume) functions is not necessarily an SCC? [1] https://reviews.llvm.org/D93828#change-Xmlqywc9evLH Full diff: https://github.com/llvm/llvm-project/pull/116285.diff 1 Files Affected:
diff --git a/llvm/lib/Analysis/LazyCallGraph.cpp b/llvm/lib/Analysis/LazyCallGraph.cpp
index 5aa36bfc36d468..724bd0ff416c8a 100644
--- a/llvm/lib/Analysis/LazyCallGraph.cpp
+++ b/llvm/lib/Analysis/LazyCallGraph.cpp
@@ -1775,8 +1775,9 @@ void LazyCallGraph::addSplitRefRecursiveFunctions(
if (F1 == F2)
continue;
Node &N2 = get(*F2);
- assert(!N1->lookup(N2)->isCall() &&
- "Edges between new functions must be ref edges");
+ assert(!N1->lookup(N2) ||
+ (!N1->lookup(N2)->isCall() &&
+ "Edges between new functions must be ref edges"));
}
}
#endif
|
I am confused by what replaceSwiftErrorOps is supposed to do and it doesn't seem to be well covered by lit-tests. At least in tree. The function appears to primarily operate on the original function, because it processes the SwiftErrorOps in Shape, collected from the unsplit function. However, it is called during cloning process of each resume function and from reading the code it seems to do something strange. After cloning the first resume funnction it may add Load and Store instructions to the original function. These would then appear in any subsequent resume functions that are cloned from the original, but not the one being processed. Instead an alloca will be created in the first resume function. After the first call to replaceSwiftErrorOps the SwiftErrorOps list is cleared so no other resume functions will get the alloca. Following this replaceSwiftErrorOps is called again after splitting but that would do nothing (right?). Removing the call within the Cloner::create() doesn't break any lit-tests. Can this be safely removed? I am looking at this because I am working on splitting. As explained in llvm#116285 I want to CloneAndPrune to create resume functions that only include the code they need and not the entire original function. However, this call causes coro-swifterror.ll to fail by: swifterror argument should come from an alloca or parameter ptr poison tail call void @maybeThrow(ptr swifterror poison) The swifterror argument is not correctly used in a few places in the IR and some how removing the replaceSwiftErrorOps call in Cloner::create() resolves the problem.
I am confused by what replaceSwiftErrorOps is supposed to do and it doesn't seem to be well covered by lit-tests. At least in tree. The function appears to primarily operate on the original function, because it processes the SwiftErrorOps in Shape, collected from the unsplit function. However, it is called during cloning process of each resume function and from reading the code it seems to do something strange. After cloning the first resume funnction it may add Load and Store instructions to the original function. These would then appear in any subsequent resume functions that are cloned from the original, but not the one being processed. Instead an alloca will be created in the first resume function. After the first call to replaceSwiftErrorOps the SwiftErrorOps list is cleared so no other resume functions will get the alloca. Following this replaceSwiftErrorOps is called again after splitting but that would do nothing (right?). Removing the call within the Cloner::create() doesn't break any lit-tests. Can this be safely removed? I am looking at this because I am working on splitting. As explained in llvm#116285 I want to CloneAndPrune to create resume functions that only include the code they need and not the entire original function. However, this call causes coro-swifterror.ll to fail by: swifterror argument should come from an alloca or parameter ptr poison tail call void @maybeThrow(ptr swifterror poison) The swifterror argument is not correctly used in a few places in the IR and some how removing the replaceSwiftErrorOps call in Cloner::create() resolves the problem.
|
I am a bit uncomfortable with general CGSCC mutation API changes. Reading the old diff, you referenced it looks like this design is deliberate. |
As far as I can see this is only used by CoroSplit, the method was introduced to add a group of continuations. There are certainly ways to work around this, but any solution is a kind of hack. For example it should be possible to add dead code that makes each resume reference all other the others (as is what happens right now). I was hoping with this PR to gain a better understanding of how CGSCC and coroutines are working together. Currently that is not well documented although I may have missed it. And perhaps find a way to avoid using hacks. |
|
Yeah, this is not the switch ABI. Let's hear the opinion from @rjmccall |
In this debug code the check assumes the lookup find an object. If it does not it tries to dereference the nullptr. This patch adds a check, however, notice it does not require the object to exist.
This method (addSplitRefRecursiveFunctions) was added for coroutines with continuations. The split coroutine transform generates the continuations by cloning the entire function then changing the entry block and a few other things. This results in a lot of dead code that is kept around only to satisfy a few assumptions then is removed.
With the dead code the original function and its continuations appear to be SCC (all functions reference all other functions). However, when the dead code is removed they are not SCC. Consider a simple coroutine { begin ... suspend ... suspend ... end }. After dead code is eliminated the ramp only references resume0, resume0 only references resume1, etc... I found the original PR [1] for addSplitRefRecursiveFunctions, but it seems to assume that the new resume functions will be a SCC.
In our use case the dead code can be a particular problem due to its size. For compile-time/size it is necessary to split the coroutine without generating dead code (e.g. CloneAndPrune). However, this also removes the dead references (on a phi's edges) to the other resume functions. The resulting resume functions are not an SCC, but this contradicts what has been written about addSplitRefRecursiveFunctions "To keep things simple, this only supports the case where all new edges are ref edges, and every new function references every other new function." [1]. The comments on are a little more relaxed saying "All new functions must reference (not call) each other." Does this mean that each resume function doesn't need to reference every other resume function?
From my testing it seems that this condition can be relaxed, and it is only this debug code that checks that "every other new function" is referenced!
Can we safely relax this such that the set of new (resume) functions is not necessarily an SCC?
[1] https://reviews.llvm.org/D93828#change-Xmlqywc9evLH