Hi Andrea, On Wed, Jul 10, 2024 at 1:47 AM Andrea Parri <parri.andrea@xxxxxxxxx> wrote: > > > > Is this second IS_ENABLED(CONFIG_RISCV_ISA_ZACAS) check actually needed? > > > (just wondering - no real objection) > > > > To me yes, otherwise a toolchain without zacas support would fail to > > assemble the amocas instruction. > > To elaborate on my question: Such a toolchain may be able to recognize > that the block of code following the zacas: label (and comprising the > amocas instruction) can't be reached/executed if the first IS_ENABLED() > evaluates to false (due to the goto end; statement), and consequently it > may compile out the entire block/instruction no matter the presence or > not of the second IS_ENABLE() check. IOW, such a toolchain/compiler may > not actually have to assemble the amocas instruction under such config. > In fact, this is how the current gcc trunk (which doesn't support zacas) > seems to behave. And this very same optimization/code removal seems to > be performed by clang when CONFIG_RISCV_ISA_ZACAS=n. IAC, I'd agree it > is good to be explicit in the sources and keep both of these checks. Indeed, clang works fine without the second IS_ENABLED(). I'll remove it then as the code is complex enough. Thanks, Alex > > > > > Why the semicolon? > > > > That fixes a clang warning reported by Nathan here: > > https://lore.kernel.org/linux-riscv/20240528193110.GA2196855@thelio-3990X/ > > I see. Thanks for the pointer. > > > > > This is because the compiler doesn't realize __ret is actually > > > initialized, right? IAC, seems a bit unexpected to initialize > > > with (old) (which indicates SUCCESS of the CMPXCHG operation); > > > how about using (new) for the initialization of __ret instead? > > > would (new) still work for you? > > > > But amocas rd register must contain the expected old value in order to > > actually work right? > > Agreed. Thanks for the clarification. > > Andrea
