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
