2022-12-16 18:39:39

by Segher Boessenkool

[permalink] [raw]
Subject: Re: [PATCH v2] powerpc: Pass correct CPU reference to assembler

On Fri, Dec 16, 2022 at 05:57:46PM +0000, Christophe Leroy wrote:
> Le 16/12/2022 ? 18:18, Segher Boessenkool a ?crit?:
> > On Fri, Dec 16, 2022 at 09:35:50AM +0100, Christophe Leroy wrote:
> >> Today we have CONFIG_TARGET_CPU which provides the identification of the
> >> expected CPU, it is used for GCC. Use it as well for the assembler.
> >
> > Why do you use -Wa, at all for this? The compiler should already pass
> > proper options always!
>
> That's historical I guess. Comes from commit 14cf11af6cf6 ("powerpc:
> Merge enough to start building in arch/powerpc.")

Ah. The patch moves stuff around, I thought more of it is new than it
really is. Sorry.

It would be good to get rid of all such things that do no good and can
easily cause problems, of course, but that does not belong to this patch
of course.

> >> +cpu-as-$(CONFIG_PPC_BOOK3S_64) += $(call as-option,-Wa$(comma)-many)
> >
> > What is this for? Using -many is a huge step back, it hides many
> > problems :-(
>
> The only thing I did is removed the -Wa,-mpower4 from the line, leaving
> the remaining part. Initialy it was:
>
> cpu-as-$(CONFIG_PPC_BOOK3S_64) += $(call as-option,-Wa$(comma)-mpower4)
> $(call as-option,-Wa$(comma)-many)
>
> It was added in 2018 by commit 960e30029863 ("powerpc/Makefile: Fix
> PPC_BOOK3S_64 ASFLAGS"). There is a long explanation it the commit.
>
> Should we remove it ?

The commit says it is a workaround for clang problems, so it needs
testing there. It also needs testing everywhere else, because as I said
it hides a lot of problems, so removing it will make a lot of sloppy
code that has crept in since 2018 scream bloody murder :-(


Segher


2022-12-17 01:26:00

by Pali Rohár

[permalink] [raw]
Subject: Re: [PATCH v2] powerpc: Pass correct CPU reference to assembler

On Friday 16 December 2022 12:10:48 Segher Boessenkool wrote:
> On Fri, Dec 16, 2022 at 05:57:46PM +0000, Christophe Leroy wrote:
> > Le 16/12/2022 à 18:18, Segher Boessenkool a écrit :
> > > On Fri, Dec 16, 2022 at 09:35:50AM +0100, Christophe Leroy wrote:
> > >> Today we have CONFIG_TARGET_CPU which provides the identification of the
> > >> expected CPU, it is used for GCC. Use it as well for the assembler.
> > >
> > > Why do you use -Wa, at all for this? The compiler should already pass
> > > proper options always!
> >
> > That's historical I guess. Comes from commit 14cf11af6cf6 ("powerpc:
> > Merge enough to start building in arch/powerpc.")
>
> Ah. The patch moves stuff around, I thought more of it is new than it
> really is. Sorry.
>
> It would be good to get rid of all such things that do no good and can
> easily cause problems, of course, but that does not belong to this patch
> of course.

Just a coincident but u-boot has similar problem...
https://patchwork.ozlabs.org/project/uboot/patch/[email protected]/

So I agree that removal of -Wa,-mXXX is a good idea. I checked that gcc
pass correct -Wa,-mXXX flag from -mcpu=YYY flag.

> > >> +cpu-as-$(CONFIG_PPC_BOOK3S_64) += $(call as-option,-Wa$(comma)-many)
> > >
> > > What is this for? Using -many is a huge step back, it hides many
> > > problems :-(
> >
> > The only thing I did is removed the -Wa,-mpower4 from the line, leaving
> > the remaining part. Initialy it was:
> >
> > cpu-as-$(CONFIG_PPC_BOOK3S_64) += $(call as-option,-Wa$(comma)-mpower4)
> > $(call as-option,-Wa$(comma)-many)
> >
> > It was added in 2018 by commit 960e30029863 ("powerpc/Makefile: Fix
> > PPC_BOOK3S_64 ASFLAGS"). There is a long explanation it the commit.
> >
> > Should we remove it ?
>
> The commit says it is a workaround for clang problems, so it needs
> testing there. It also needs testing everywhere else, because as I said
> it hides a lot of problems, so removing it will make a lot of sloppy
> code that has crept in since 2018 scream bloody murder :-(
>
>
> Segher