[KeyInstr][Clang] Ret atom

[OCHyams]

[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

[OCHyams]

Ready for review

• Add parameter RetKeyInstructionsSourceAtom in EmitFunctionEpilog to request a ret gets a specific atomGroup number through addInstToSpecificSourceAtom

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.

[jmorse]

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).

[jmorse]

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"?

[OCHyams]

The code below has since been added in another patch that's already landed (without the comment) - rebased to remove from diff

[jmorse]

Why's this memcpy getting the same atom group as the return?

[OCHyams]

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.

[jmorse]

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?

[OCHyams]

Yep in Clang - here

llvm-project/clang/lib/CodeGen/CodeGenFunction.cpp

Line 336 in 2e70da3

if (ReturnBlock.getBlock()->hasOneUse()) {

[OCHyams]

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).

[OCHyams]

The code below has since been added in another patch that's already landed (without the comment) - rebased to remove from diff

[OCHyams]

Yep in Clang - here

llvm-project/clang/lib/CodeGen/CodeGenFunction.cpp

Line 336 in 2e70da3

if (ReturnBlock.getBlock()->hasOneUse()) {

[OCHyams]

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.

[jmorse]

LGTM