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

[nickdesaulniers]

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

[nickdesaulniers]

• update LanguageExtensions doc

[void]

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]

In D136497#3889938, @void wrote:

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.

[nickdesaulniers]

clang/lib/CodeGen/CGStmt.cpp

2859–2860

Now done in D137113.

[nickdesaulniers]

• rebase on D137113, add comment

[nickdesaulniers]

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]

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.

[nickdesaulniers]

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.

[nickdesaulniers]

• remove unnecssary asserts

[nickdesaulniers]

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]

• 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]

• rebase, use new callbrpad inst

[nickdesaulniers]

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

[nickdesaulniers]

• rebase, drop callbrpad inst

[nickdesaulniers]

• add clang release notes from D138078 and new one for new feature.

[nickdesaulniers]

Ready for review:

1. https://reviews.llvm.org/D135997
2. https://reviews.llvm.org/D140166
3. https://reviews.llvm.org/D139861
4. https://reviews.llvm.org/D139872
5. https://reviews.llvm.org/D139883
6. https://reviews.llvm.org/D139970
7. https://reviews.llvm.org/D140160
8. https://reviews.llvm.org/D137113
9. https://reviews.llvm.org/D136497

[nickdesaulniers]

• rebase, format

[nickdesaulniers]

• rebase

[void]

clang/docs/ReleaseNotes.rst

64

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]

• use densemap, update release note, use !empty rather than size as per @void

[efriedma]

clang/docs/LanguageExtensions.rst

1592

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]

One small comment about the documentation but L:GTM.

clang/docs/LanguageExtensions.rst

1592

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.

[nickdesaulniers]

clang/docs/LanguageExtensions.rst

1592

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]

• rebase

[nickdesaulniers]

• final rebase

[uabelho]

I noticed that gvn-hoist doesn't handle this very nicely, I just wrote https://github.com/llvm/llvm-project/issues/61023