Re: [PATCH] Kconfig: Explicitly disable asm goto w/ outputs on gcc-11 (and earlier)

[Nick Desaulniers]

On Fri, Feb 9, 2024 at 11:01 AM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, 9 Feb 2024 at 10:55, Nick Desaulniers <ndesaulniers@xxxxxxxxxx> wrote:
> >
> > And also pessimizes all asm gotos (for gcc) including ones that don't
> > contain output as described in 43c249ea0b1e.  The version checks would
> > at least not pessimize those.
>
> Yeah, no, we should limit that workaround only to the asm goto with
> outputs case.
>
> We should also probably get rid of the existing "asm_volatile_goto()"
> macro name entirely. That name was always pretty horrific, in that it
> didn't even mark the asm as volatile even in the case where it did
> anything.

+1.

43c249ea0b1e could have done so, but I'm guessing "tree wide changes
in Linux are not fun" was perhaps the reason it wasn't done so then.

I also think folks are too aggressive putting volatile on asm
statements that might not need them; it's definitely less cognitive
burden to just always put `volatile` on inline asm but I suspect
that's leaving some performance on the floor in certain cases.  (I had
a patch to clang to warn when the volatile was unnecessary in cases
where it was explicit, but that was shot down in code review as being
harassing to users).

>
> So the name of that macro made little sense, and the new workaround
> should be only for asm goto with outputs. So I'd suggest jmaking a new
> macro with that name:
>
>    #define asm_goto_output(x...)
>
> and on gcc use that old workaround, and on clang just make it be a
> plain "asm goto".
>
> Hmm?

Thinking through the tradeoffs, the Kconfig approach would pessimize
GCC versions with the "lack of implicit volatile" bug to not use asm
goto w/ outputs at all.

Having a new macro would just make the `volatile` qualifier explicit,
which is a no-op on gcc versions that don't contain the bug (while
allowing them to use asm goto with outputs, which is probably better
for codegen).

So I agree a new macro seems better than disabling things for kconfig.
(Assuming the only issue is the need to make `volatile` explicit for a
few GCC versions; it's not clear to me from Sean's latest response if
there's more than one bug here).  I'm not signing up to shave either
yak though.
-- 
Thanks,
~Nick Desaulniers