[Coroutines] Remove assert about a promise being present #156007
Conversation
This commit removes an assert in the generation of debug info for a coroutine frame. This assert checked if a promise alloca is present, even though it's not used. While this might always be the case when the coroutine was produced by clang++, this doesn't hold in the general case.
Dinistro
requested review from
ChuanqiXu9,
gysit,
kazutakahirata and
vogelsgesang
|
@llvm/pr-subscribers-coroutines Author: Christian Ulmann (Dinistro) ChangesThis commit removes an assert in the generation of debug info for a coroutine frame. This assert checked if a promise alloca is present, even though it's not used. While this might always be the case when the coroutine was produced by clang++, this doesn't hold in the general case. Full diff: https://github.com/llvm/llvm-project/pull/156007.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index b775c43460190..08f03aa45255d 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -700,9 +700,6 @@ static void buildFrameDebugInfo(Function &F, coro::Shape &Shape,
DIBuilder DBuilder(*F.getParent(), /*AllowUnresolved*/ false);
- assert(Shape.getPromiseAlloca() &&
- "Coroutine with switch ABI should own Promise alloca");
-
DIFile *DFile = DIS->getFile();
unsigned LineNum = DIS->getLine();
diff --git a/llvm/test/Transforms/Coroutines/coro-split-dbg.ll b/llvm/test/Transforms/Coroutines/coro-split-dbg.ll
index 9a9e3c3f2bf0e..02bd2b2d0d65f 100644
--- a/llvm/test/Transforms/Coroutines/coro-split-dbg.ll
+++ b/llvm/test/Transforms/Coroutines/coro-split-dbg.ll
@@ -77,7 +77,7 @@ attributes #9 = { noduplicate }
!llvm.module.flags = !{!3, !4}
!llvm.ident = !{!5}
-!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 4.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 4.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
!1 = !DIFile(filename: "coro.c", directory: "/home/gor/build/bin")
!2 = !{}
!3 = !{i32 2, !"Dwarf Version", i32 4}
|
|
@llvm/pr-subscribers-llvm-transforms Author: Christian Ulmann (Dinistro) ChangesThis commit removes an assert in the generation of debug info for a coroutine frame. This assert checked if a promise alloca is present, even though it's not used. While this might always be the case when the coroutine was produced by clang++, this doesn't hold in the general case. Full diff: https://github.com/llvm/llvm-project/pull/156007.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index b775c43460190..08f03aa45255d 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -700,9 +700,6 @@ static void buildFrameDebugInfo(Function &F, coro::Shape &Shape,
DIBuilder DBuilder(*F.getParent(), /*AllowUnresolved*/ false);
- assert(Shape.getPromiseAlloca() &&
- "Coroutine with switch ABI should own Promise alloca");
-
DIFile *DFile = DIS->getFile();
unsigned LineNum = DIS->getLine();
diff --git a/llvm/test/Transforms/Coroutines/coro-split-dbg.ll b/llvm/test/Transforms/Coroutines/coro-split-dbg.ll
index 9a9e3c3f2bf0e..02bd2b2d0d65f 100644
--- a/llvm/test/Transforms/Coroutines/coro-split-dbg.ll
+++ b/llvm/test/Transforms/Coroutines/coro-split-dbg.ll
@@ -77,7 +77,7 @@ attributes #9 = { noduplicate }
!llvm.module.flags = !{!3, !4}
!llvm.ident = !{!5}
-!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 4.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 4.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
!1 = !DIFile(filename: "coro.c", directory: "/home/gor/build/bin")
!2 = !{}
!3 = !{i32 2, !"Dwarf Version", i32 4}
|
| assert(Shape.getPromiseAlloca() && | ||
| "Coroutine with switch ABI should own Promise alloca"); | ||
|
|
There was a problem hiding this comment.
Another possibility is to add some new kind of MD that indicates the coroutine was directly generated at the IR level (in which case, we allow the promise not to exist). Then, we can leave this assert intact for regular C++ coroutines.
There was a problem hiding this comment.
While that might be possible, it seems odd that its there in the first place. This is not a pre-condition of this function. Also, relying on metadata is a bit dangerous, as it is not guaranteed to be preserved until this pass is executed.
There was a problem hiding this comment.
Looking at the last commit that touched the assert it seems this is probably a leftover from a refactoring that removed the logic to get the debug information from a debug declare (record/intrinsic).
ChuanqiXu9
left a comment
ChuanqiXu9
left a comment
There was a problem hiding this comment.
I am curious about your downstream project using LLVM's coroutine intrinsics.
We have a complicated flow that I cannot fully disclose here. What is relevant is that we take some input programs without coroutines and convert functions in there to coroutines. Once some IP questions have been resolved, we might publish our use case. |
Looking forward for your results : ) |
This commit removes an assert in the generation of debug info for a coroutine frame. This assert checked if a promise alloca is present, even though it's not used. While this might always be the case when the coroutine was produced by clang++, this doesn't hold in the general case.
Note: We generate coroutine intrinsics from downstream passes. In our case, there is no guarantee that a coroutine has any promise, but they can originate from some non-coro C++ code.