What sort of outputs, specifically? Memory outputs are easy; I think the LLVM IR patch actually supports them already. Register outputs are complicated... I guess they're also possible, but we would need to come up with some frontend rule that allows sane code generation. Consider the case where two or more asm gotos jump to the same destination, and output a value to the same register, but a different variable. In general, it's possible to lower by rewriting the PHI nodes to match, then inserting a switch at the beginning of the destination to fix the rewritten PHIs. But that's both expensive at runtime and difficult to implement, so we probably want a rule that prohibits constructs which would require that sort of lowering.
If we do intend to implement register outputs at some point, we might want to draft a design at the LLVM IR level now, so we have some confidence the current IR is forward-compatible. Obviously we should have diagnostics in the parser if the user tries to write an asm goto with an output. I'm not worried about the difficulty of fixing the frontend otherwise; this patch isn't that big in the first place. -Eli ________________________________ From: Nick Desaulniers <ndesaulni...@google.com> Sent: Tuesday, February 5, 2019 1:14 AM To: reviews+d56571+public+1482c3abd76a8...@reviews.llvm.org Cc: Yu, Jennifer; Keane, Erich; chandl...@gmail.com; syaghm...@apple.com; craig.top...@gmail.com; h...@chromium.org; e5ten.a...@gmail.com; d...@golovin.in; natechancel...@gmail.com; tstel...@redhat.com; Eli Friedman; srhi...@google.com; era...@google.com; cfe-commits@lists.llvm.org Subject: [EXT] Re: [PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang Note that kernel developers are requesting output support with ASM goto. Doesn't need to be in V1 of ASM go-to support, but we should expect it to be implemented in the future. We should help users with diagnostics that this is not supported, and add asserts that will help us implement such a feature in the future. On Tue, Feb 5, 2019, 3:32 AM Richard Smith - zygoloid via Phabricator <revi...@reviews.llvm.org<mailto:revi...@reviews.llvm.org> wrote: rsmith added inline comments. ================ Comment at: lib/AST/Stmt.cpp:457-460 this->NumOutputs = NumOutputs; this->NumInputs = NumInputs; this->NumClobbers = NumClobbers; + this->NumLabels = NumLabels; ---------------- jyu2 wrote: > rsmith wrote: > > Please assert that you don't have both outputs and labels here. (If you > > did, you would assign them the same slots within `Exprs`.) > > > > You also seem to be missing `Sema` enforcement of the rule that you cannot > > have both outputs and labels. (If you want to actually support that as an > > intentional extension to the GCC functionality, you should change the > > implementation of `GCCAsmStmt` to use different slots for outputs and > > labels, add some tests, and add a `-Wgcc-compat` warning for that case. > > Seems better to just add an error for it for now.) > This is enforcement during the parer. > > for example: > a.c:12:23: error: expected ':' > asm goto ("decl %0;" :"a": "m"(cond) : "memory" ); > > Do you think this is enough for now? > Thanks. > Jennifer Thanks, I see it now. Please still add the assert here. I'd also like a custom diagnostic for that parse error; it'd seem easy and useful to add an "asm goto cannot have output constraints" error. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56571/new/ https://reviews.llvm.org/D56571
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
