[asm goto] support outputs along indirect branches

[josephcsible]

Consider this C code:

int main(void) {
    int x = 123;
    asm goto("mov %1, %0\n\tjmp %l[label]" : "=r" (x) : "r" (45) : : label);
    x = 6;
    label:
    return x;
}

It's supposed to return 45. But when you compile it in Clang, it returns 123 instead. I tested this on both trunk (01b52f7) and 13.0.0, and both with -O3 and without optimizations, and the bug happens in all of those cases.

https://godbolt.org/z/vKeYrfYY5

[llvmbot]

@llvm/issue-subscribers-clang-codegen

[efriedma-quic]

Outputs are only valid on the fallthrough codepath; on other paths, the output values are ignored. That's by design. Or at least, it's a practical consequence of the way asm goto is currently implemented.

This is documented at https://clang.llvm.org/docs/LanguageExtensions.html#asm-goto-with-output-constraints . -Wuninitialized prints a warning for your testcase. The warning could probably be improved. Not sure what else we can do here.

[josephcsible]

Okay, I see what happened. This part of that documentation is no longer quite accurate:

In addition to the functionality provided by GCC’s extended assembly, clang supports output constraints with the goto form.

The goto form of GCC’s extended assembly allows the programmer to branch to a C label from within an inline assembly block. Clang extends this behavior by allowing the programmer to use output constraints:

Because as of version 11.1.0 (specifically, commit e3b3b59), GCC now supports asm goto with outputs, without a restriction to only the fallthrough codepath. So now we support only a subset of their functionality rather than an extension of it. In light of this, can this issue be reconsidered as a feature request to match that functionality?

[nickdesaulniers]

cc @jyknight @gwelymernans @isanbard

can this issue be reconsidered as a feature request to match that functionality?

Yeah, we probably can't focus on this for a couple months, but this is surely a portability issue that folks will begin to notice more and more frequently.

We'll probably need to undo parts of https://reviews.llvm.org/D79794, making INLINEASM_BR a terminator again. We'll also probably need to split critical edges to insert COPYs for the indirect target. I forget now why we didn't; I think critical edge splitting is fallible.

Also, there is one undesirable side effect where address of label would no longer refer to the same label as the asm goto IIRC. But I'd say support for outputs along all edges is probably more worthwhile than such a minute detail.

[isanbard]

If GCC's able to allow outputs on indirect branches, then they must not care about the label addresses like we did. (Segher mentioned something along those lines in our discussions with him before the implementation.) That would take care of the vast majority of issues we faced with splitting critical edges.

I don't know if it's worth making INLINEASM_BR a terminator again, since that has its own issues. We'll have to see though.

[nickdesaulniers]

https://discourse.llvm.org/t/rfc-syncing-asm-goto-with-outputs-with-gcc/65453/4

[nickdesaulniers]

Updating docs to make note of this difference https://reviews.llvm.org/D135818. Will need to revisit those docs when this is implemented.

[nickdesaulniers]

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]

Landed as:

1. commit 45a291b ("[Dominators] check indirect branches of callbr")
2. commit fb47115 ("[llvm] boilerplate for new callbrprepare codegen IR pass")
3. commit 0a39af0 ("[llvm][CallBrPrepare] split critical edges")
4. commit 094190c ("[llvm][CallBrPrepare] add llvm.callbr.landingpad intrinsic")
5. commit 28d45c8 ("[llvm][CallBrPrepare] use SSAUpdater to use intrinsic value")
6. commit 5cc1016 ("[llvm][SelectionDAGBuilder] codegen callbr.landingpad intrinsic")
7. commit a3a84c9 ("[llvm] add CallBrPrepare pass to pipelines")
8. commit b1bc723 ("[Clang] refactor CodeGenFunction::EmitAsmStmt NFC")
9. commit 329ef60 ("[Clang] support for outputs along indirect edges of asm goto")
10. commit af66563 ("[clang] fix -Wuninitialized for asm goto outputs on indirect edges.")
11. commit 54186d3 ("[clang] add __has_extension(gnu_asm_goto_with_outputs_full)")
12. commit 39811e2 ("[llvm][test] enable/disable -verify-machineinstrs where possible for callbr")
13. commit cf86855 ("[M68k] fix test regression introduced by D140180")

This is intentionally going to ship in clang-17; I want it to miss the clang-16 train so that it gets maximal soak time. @josephcsible please help test using ToT. @nathanchance is going to help me keep an eye on this in case we need to revert/back it out (hence the above list, in case it's needed). Please file new bugs and assign to me for issues found with this feature.

[bwendling]

Huzzah!!! Great job. :-)

[josephcsible]

@josephcsible please help test using ToT.

I broke it with the exact same code as in my first post here ☹️ #60855

[nickdesaulniers]

Another follow up reported issue: #61023