[KeyInstr][Clang] Ret atom #134652
[KeyInstr][Clang] Ret atom #134652
Conversation
|
Keeping in draft till the patches below this in the stack are merged, as this touches all their tests. Unfortunately the diff is going to be messed up till then too due to various branch retargeting and rebasing. |
CGDebugInfo::completeFunction was added previously but mistakenly not called (dropped through the cracks while putting together the patch stack). Moved out of llvm#134652 and llvm#134654.
CGDebugInfo::completeFunction was added previously but mistakenly not called (dropped through the cracks while putting together the patch stack). Moved out of #134652 and #134654. This patch is part of a stack that teaches Clang to generate Key Instructions metadata for C and C++. RFC: https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668 The feature is only functional in LLVM if LLVM is built with CMake flag LLVM_EXPERIMENTAL_KEY_INSTRUCTIONs. Eventually that flag will be removed.
a13e2d5 to
93a95dd
Compare
There was a problem hiding this comment.
Ready for review
- Add parameter
RetKeyInstructionsSourceAtominEmitFunctionEpilogto request aretgets a specific atomGroup number throughaddInstToSpecificSourceAtom
Most of the updated tests just check for any atom on the ret, but the two tests added in this PR check for specific atom numbers.
There was a problem hiding this comment.
If I understand correctly, most of the test changes are to ensure that return instructions get /an/ atom group, without being specific about which? IMO it'd be better to make that number explicit to avoid a return accidentally being part of the wrong atom group. It's necessary for it to be a distinct group, right? (A chore, but an important fact).
clang/lib/CodeGen/CGCleanup.cpp
Outdated
| @@ -1118,6 +1118,8 @@ void CodeGenFunction::EmitBranchThroughCleanup(JumpDest Dest) { | ||
|
|
||
| // Create the branch. | ||
| llvm::BranchInst *BI = Builder.CreateBr(Dest.getBlock()); | ||
| // This is the primary instruction for this atom, acting in place of a ret. |
There was a problem hiding this comment.
I think in isolation this comment will be hard to understand, particularly relating it to a ret. Perhaps "this atom" -> "the source atom leaving this scope"?
There was a problem hiding this comment.
The code below has since been added in another patch that's already landed (without the comment) - rebased to remove from diff
| __builtin_va_start(list, z); | ||
| // CHECK: vaarg.end: | ||
| // CHECK-NEXT: %vaarg.addr = phi ptr | ||
| // CHECK-NEXT: call void @llvm.memcpy{{.*}}, !dbg [[G1R1:!.*]] |
There was a problem hiding this comment.
Why's this memcpy getting the same atom group as the return?
There was a problem hiding this comment.
The comment from test return.c covers this case:
// Check the stores to `retval` allocas and branches to `return` block are in
// the same atom group. They are both rank 1, which could in theory introduce
// an extra step in some optimized code. This low risk currently feels an
// acceptable for keeping the code a bit simpler (as opposed to adding
// scaffolding to make the store rank 2).
(But in this case the "store to retval" is a memcpy).
We want them in the same group because these are all instructions generated from the source return some_value.
| // Also check that in the case of a single return (no control flow) the | ||
| // return instruction inherits the atom group of the branch to the return | ||
| // block when the blocks get folded togather. |
There was a problem hiding this comment.
Could you remark on which test function this is, as I couldn't spot it. Presumably the folding happens in clang, rather than being something done by LLVM?
There was a problem hiding this comment.
Yep in Clang - here
This patch is part of a stack that teaches Clang to generate Key Instructions metadata for C and C++. The Key Instructions project is introduced, including a "quick summary" section at the top which adds context for this PR, here: https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668 The feature is only functional in LLVM if LLVM is built with CMake flag LLVM_EXPERIMENTAL_KEY_INSTRUCTIONs. Eventually that flag will be removed. The Clang-side work is demoed here: #130943
…d fixup some tests
There was a problem hiding this comment.
If I understand correctly, most of the test changes are to ensure that return instructions get /an/ atom group, without being specific about which? IMO it'd be better to make that number explicit to avoid a return accidentally being part of the wrong atom group. It's necessary for it to be a distinct group, right? (A chore, but an important fact).
Done. Rebased (to address an inline comment and capture since-merged tests).
clang/lib/CodeGen/CGCleanup.cpp
Outdated
| @@ -1118,6 +1118,8 @@ void CodeGenFunction::EmitBranchThroughCleanup(JumpDest Dest) { | ||
|
|
||
| // Create the branch. | ||
| llvm::BranchInst *BI = Builder.CreateBr(Dest.getBlock()); | ||
| // This is the primary instruction for this atom, acting in place of a ret. |
There was a problem hiding this comment.
The code below has since been added in another patch that's already landed (without the comment) - rebased to remove from diff
| // Also check that in the case of a single return (no control flow) the | ||
| // return instruction inherits the atom group of the branch to the return | ||
| // block when the blocks get folded togather. |
There was a problem hiding this comment.
Yep in Clang - here
| __builtin_va_start(list, z); | ||
| // CHECK: vaarg.end: | ||
| // CHECK-NEXT: %vaarg.addr = phi ptr | ||
| // CHECK-NEXT: call void @llvm.memcpy{{.*}}, !dbg [[G1R1:!.*]] |
There was a problem hiding this comment.
The comment from test return.c covers this case:
// Check the stores to `retval` allocas and branches to `return` block are in
// the same atom group. They are both rank 1, which could in theory introduce
// an extra step in some optimized code. This low risk currently feels an
// acceptable for keeping the code a bit simpler (as opposed to adding
// scaffolding to make the store rank 2).
(But in this case the "store to retval" is a memcpy).
We want them in the same group because these are all instructions generated from the source return some_value.
37c869a to
689c1b1
Compare
CGDebugInfo::completeFunction was added previously but mistakenly not called (dropped through the cracks while putting together the patch stack). Moved out of llvm#134652 and llvm#134654. This patch is part of a stack that teaches Clang to generate Key Instructions metadata for C and C++. RFC: https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668 The feature is only functional in LLVM if LLVM is built with CMake flag LLVM_EXPERIMENTAL_KEY_INSTRUCTIONs. Eventually that flag will be removed.
[KeyInstr][Clang] Ret atom
This patch is part of a stack that teaches Clang to generate Key Instructions
metadata for C and C++.
The Key Instructions project is introduced, including a "quick summary" section
at the top which adds context for this PR, here:
https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668
The feature is only functional in LLVM if LLVM is built with CMake flag
LLVM_EXPERIMENTAL_KEY_INSTRUCTIONs. Eventually that flag will be removed.
The Clang-side work is demoed here:
#130943
[KeyInstr][Clang] Update tests with ret atoms