Page MenuHomePhabricator

[Clang] support for outputs along indirect edges of asm goto
AcceptedPublic

Authored by nickdesaulniers on Oct 21 2022, 2:08 PM.

Details

Summary

Initial support for asm goto w/ outputs (D69876) only supported outputs
along the "default" (aka "fallthrough") edge.

We can support outputs along all edges by repeating the same pattern of
stores along the indirect edges that we allready do for the default
edge. One complication is that these indirect edges may be critical
edges which would need to be split. Another issue is that mid-codgen of
LLVM IR, the control flow graph might not reflect the control flow of
the final function.

To avoid this "chicken and the egg" problem assume that any given
indirect edge may become a critical edge, and pro-actively split it.
This is unnecessary if the edge does not become critical, but LLVM will
optimize such cases via tail duplication.

Fixes: https://github.com/llvm/llvm-project/issues/53562

Diff Detail

Unit TestsFailed

TimeTest
3,820 mslibcxx CI Modules > llvm-libc++-shared-cfg-in.libcxx/algorithms/specialized_algorithms/special_mem_concepts::nothrow_sentinel_for.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /home/libcxx-builder/.buildkite-agent/builds/db0bd8c6e91b-1/llvm-project/libcxx-ci/install/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/db0bd8c6e91b-1/llvm-project/libcxx-ci/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_sentinel_for.compile.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/db0bd8c6e91b-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/db0bd8c6e91b-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/db0bd8c6e91b-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fmodules -fcxx-modules -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -fsyntax-only

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2022, 2:08 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
nickdesaulniers requested review of this revision.Oct 21 2022, 2:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2022, 2:08 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nickdesaulniers edited the summary of this revision. (Show Details)
  • add note to commit message (let's see if phab captures what I wrote, used arc diff --verbatim)
  • git clang-format HEAD~

Note to reviewers: I expect this to be one of a 3 part progression.

  1. Fix DomTree D135997
  2. Fix front end D136497
  3. Fix back end (TODO)

Hopefully that's it; famous last words...

  • update LanguageExtensions doc
void added a comment.Oct 27 2022, 2:49 PM

It might be easier to see the main changes here if you submit the (very nice) refactoring of EmitAsmStores first.

clang/lib/CodeGen/CGStmt.cpp
2361

s/ResultTruncRegTypes[i]/TruncTy/

2365

This looks like a direct copy from below (which is fine). I'm a bit iffy on whether this covers *all* of the value types that could come through here...

2859–2860

Should these asserts (or some of them) be moved into EmitAsmStores()?

2863

nit: Could you add a comment explaining that we're calling EmitAsmStores for asm goto's indirect branches? I was slightly confused about why we were calling EmitAsmStores more than once. :)

nickdesaulniers planned changes to this revision.Oct 31 2022, 2:20 PM

It might be easier to see the main changes here if you submit the (very nice) refactoring of EmitAsmStores first.

D137113 TODO(Nick): rebase on D137113

clang/lib/CodeGen/CGStmt.cpp
2361

Done in D137113

2365

Note: this code was simply moved wholesale. If value types are missing, that is a pre-existing bug and should be a prerequisite to fix for this patch.

I've broken out D137113 as a child patch to make that clearer.

2365

s/should/shouldn't/

2859–2860

Ah, yeah, let me add those to D137113.

clang/lib/CodeGen/CGStmt.cpp
2859–2860

Now done in D137113.

nickdesaulniers planned changes to this revision.Oct 31 2022, 2:46 PM
nickdesaulniers added inline comments.
clang/lib/CodeGen/CGStmt.cpp
2869

Oh, I can remove this now, I moved it into EmitAsmStores.

2870–2875

I'm not sure that I need to handle this case (of a callbr with the same indirect destination as the default destination). That might result from optimizations, but I don't think clang can or will generate such IR. Maybe I should turn this into an: assert(CBR->getIndirectDest(i) != CBR->getDefaultDest(i) && "We already emitted asm stores in the default dest");?

void added inline comments.Oct 31 2022, 2:56 PM
clang/lib/CodeGen/CGStmt.cpp
2870–2875

It could only happen with something like this:

asm goto (... :::: indirect);

indirect:;

In that simplified case, it looks like clang emits this:

define dso_local void @foo() #0 {
  callbr void asm sideeffect "", "i,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@foo, %2)) #1
          to label %1 [label %2], !srcloc !6

1:                                                ; preds = %0
  br label %2

2:                                                ; preds = %1, %0
  ret void
}

So it's probably safe to make this an assert.

clang/lib/CodeGen/CGStmt.cpp
2870–2875

Yeah, it's impossible. While a CallBrInst can have a same indirect destination as the default destination as a result of optimizations, above we always create an "asm.fallthrough" block that is distinct from any of the named labels from the asm goto.

  • remove unnecssary asserts
clang/lib/CodeGen/CGStmt.cpp
2870–2875

Oh, it's not just the default and indirects sharing, but what if the same indirect is listed twice? I should add a test for that...i.e.

asm goto (... :::: indirect,indirect);
nickdesaulniers planned changes to this revision.Oct 31 2022, 3:06 PM
nickdesaulniers marked an inline comment as done.
  • reroll for a new sha so that phab unmarks this as "changes planned"
clang/lib/CodeGen/CGStmt.cpp
2870–2875

Oh, right, we don't allow that.

error: duplicate use of asm operand name "indirect"
note: asm operand name "indirect" first referenced here

I'm going to leave these asserts out and mark this thread done, but please reopen this thread with a new comment if you feel strongly that I should add it back (assert(CBR->getIndirectDest(i) != CBR->getDefaultDest(i) && "We already emitted asm stores in the default dest");).

nickdesaulniers edited the summary of this revision. (Show Details)
  • rebase, use new callbrpad inst

Probably should copy the release note change from https://reviews.llvm.org/D138078.

nickdesaulniers edited the summary of this revision. (Show Details)
  • rebase, drop callbrpad inst
nickdesaulniers planned changes to this revision.Dec 21 2022, 2:10 PM
  • add clang release notes from D138078 and new one for new feature.
  • rebase, format
void added inline comments.Wed, Jan 18, 4:04 PM
clang/docs/ReleaseNotes.rst
199

Is it worth it to mention that this change makes Clang's behavior with regards to your example here consistent with GCC's behavior?

clang/lib/CodeGen/CGStmt.cpp
2815

It seems as if you really want a map indexed by the BasicBlock*. So something like this:

DenseMap<BasicBlock *, SmallVector<Value *, 4>> // 4 should be enough for anyone.

That way you can iterate over CBR->getIndirectDests() instead of having to use an iterator.

2867

Could probably use !CBRRegResults.empty() instead of .size(). The first should be O(1) (granted .size() probably should be too, but .empty() is more explicit anyway).

nickdesaulniers planned changes to this revision.Thu, Jan 19, 10:06 AM
nickdesaulniers marked 3 inline comments as done.
  • use densemap, update release note, use !empty rather than size as per @void
efriedma added inline comments.Wed, Jan 25, 9:28 AM
clang/docs/LanguageExtensions.rst
1584

Maybe put a note about earlier releases, so people don't get confused if they use newer documentation than their version of clang.

Is there any chance we want a dedicated __has_extension flag?

void accepted this revision.Tue, Jan 31, 5:02 PM

One small comment about the documentation but L:GTM.

clang/docs/LanguageExtensions.rst
1584

Is there any chance we want a dedicated __has_extension flag?

Might not be a bad idea just in case they have more than one compiler version they're using. Then again, if they add code to use this feature, they're fairly committed to the assumption that it'll "just work" even with a __has_extension flag...(Remember that we currently don't actually warn if they're using the outputs on the indirect branch. We just specify that such behavior is undefined.)

I think a strongly worded note here that it's available with Clang 16+ (or whatever) is going to be slightly better. It'll force them to use macros to support multiple compiler versions, making their code wicked ugly, and them possibly coming up with better ways to ensure that the correct compiler is used for this feature.

This revision is now accepted and ready to land.Tue, Jan 31, 5:02 PM
nickdesaulniers marked 2 inline comments as done.Thu, Feb 2, 11:23 AM
nickdesaulniers added inline comments.
clang/docs/LanguageExtensions.rst
1584

I've added that as a separate commit https://reviews.llvm.org/D143205 to not hold this up. PTAL. Open to better names for the identifier.

nickdesaulniers marked an inline comment as done.Thu, Feb 2, 11:23 AM