[Bug target/101393] PowerPC32 inline assembly broken by commit 2d94f7dea9c73ef3c116a0ddc722724578a860fe
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101393 --- Comment #17 from Segher Boessenkool --- (In reply to Franz Sirl from comment #15) > > "7400" and "403" are not supported target attribute values, fwiw (says the > > manual). > > Hmm, I don't understand what you mean. I mean that I cannot read :-) You wrote "cpu=403" of course, which should be fine (as opposed to just "403".
[Bug target/101393] PowerPC32 inline assembly broken by commit 2d94f7dea9c73ef3c116a0ddc722724578a860fe
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101393 --- Comment #16 from Franz Sirl --- Created attachment 51199 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51199=edit Patch version with minimum changes against GCC10 This is the minimum version of the patch, it fixes this PR but still avoids code duplication. If this patch is acceptable, I'll fixup the formatting for submission.
[Bug target/101393] PowerPC32 inline assembly broken by commit 2d94f7dea9c73ef3c116a0ddc722724578a860fe
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101393
--- Comment #15 from Franz Sirl ---
(In reply to Segher Boessenkool from comment #14)
> (In reply to Franz Sirl from comment #12)
> > The emitted .machine is easy to fix, what's not so easy to fix is the
> > intention behind Segher's change that caused the wrong .machine.
> >
> > Consider this source compiled with -mcpu=7400:
> >
> > void ppcaltivecfunc (void) __attribute__ ((__target__
> > ("cpu=7400,altivec")));
> > void ppcaltivecfunc (void)
> > {
> > asm ("lvx 0,0,11");
> > }
> >
> >
> > void ppc403func (void) __attribute__ ((__target__ ("cpu=403")));
> > void ppc403func (void)
> > {
> > asm ("lvx 0,0,11");
> > }
>
> "7400" and "403" are not supported target attribute values, fwiw (says the
> manual).
Hmm, I don't understand what you mean. As I read the manual any -mcpu= option
is allowed? And certainly this seems to do something as you can see in the
assembler source. The only strange thing is that -mppc is passed to the
assembler for -mcpu=7400, but in the assembler source it selects ".machine
power7"?
> > That's why I suggested more control over the sticky flags via .machine
> > pseudo-ops, -mdotmachine-realcpu-resets-sticky or similar. That way the
> > users of pure assembly sources won't see a change in behaviour, but the
> > compiler still gains the full control it needs.
>
> We should just make this stuff work better / more intuitively :-) We do
> not need to preserve unworkable (or buggy) semantics, in general.
In that case I'll send a minimum patch for now.
[Bug target/101393] PowerPC32 inline assembly broken by commit 2d94f7dea9c73ef3c116a0ddc722724578a860fe
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101393
--- Comment #14 from Segher Boessenkool ---
(In reply to Franz Sirl from comment #12)
> The emitted .machine is easy to fix, what's not so easy to fix is the
> intention behind Segher's change that caused the wrong .machine.
>
> Consider this source compiled with -mcpu=7400:
>
> void ppcaltivecfunc (void) __attribute__ ((__target__ ("cpu=7400,altivec")));
> void ppcaltivecfunc (void)
> {
> asm ("lvx 0,0,11");
> }
>
>
> void ppc403func (void) __attribute__ ((__target__ ("cpu=403")));
> void ppc403func (void)
> {
> asm ("lvx 0,0,11");
> }
"7400" and "403" are not supported target attribute values, fwiw (says the
manual).
> Even though the compiler emitted a .machine "403" (or currently the buggy
> "ppc"), the assembler won't barf on "lvx 0,0,11" in ppc403func(), because of
> the sticky altivec flag.
Yeah, we should fix that.
> That's why I suggested more control over the sticky flags via .machine
> pseudo-ops, -mdotmachine-realcpu-resets-sticky or similar. That way the
> users of pure assembly sources won't see a change in behaviour, but the
> compiler still gains the full control it needs.
We should just make this stuff work better / more intuitively :-) We do
not need to preserve unworkable (or buggy) semantics, in general.
[Bug target/101393] PowerPC32 inline assembly broken by commit 2d94f7dea9c73ef3c116a0ddc722724578a860fe
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101393 Segher Boessenkool changed: What|Removed |Added Assignee|unassigned at gcc dot gnu.org |segher at gcc dot gnu.org --- Comment #13 from Segher Boessenkool --- (In reply to Alan Modra from comment #11) > Given the number of ppc micros around with differing functional units, it is > quite reasonable that we have assembly options to say "this base cpu" plus > "this extra functionality". Certainly. > Whether you think it was wise to allow those > "extra functionality" options to be specified before the "base cpu" option > is another matter, but it has been that way for a long time. We do the same in GCC, it works fine. But that works because it is treated independently of -mcpu= (our analogue of .machine). Such options are evaluated separately, always override what -mcpu= says. > I'm not > inclined to change that since it would very likely break some projects, and > I happen to think that it is entirely reasonable to expect that "-maltivec > -mppc64" for example is the same as "-mppc64 -maltivec". Very much so :-) > In any case sticky options are a side issue here. The real issue is that > emitted .machine is wrong. See my comment 2, so we agree. I'll work on it. > Note that I'm not vetoing assembler changes. It > might be a good idea to reset all sticky options on processing any .machine > directive for example, and I'm happy with the idea of -mno-vsx etc. options > for the assembler. Yeah, I don't see how it can work with push and pop and stuff this way. Ideally that would work just as if you had the options it pushes and pops on the command line, at every stage -- and AFAICS that means you have to track these options separately.
[Bug target/101393] PowerPC32 inline assembly broken by commit 2d94f7dea9c73ef3c116a0ddc722724578a860fe
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101393
--- Comment #12 from Franz Sirl ---
The emitted .machine is easy to fix, what's not so easy to fix is the intention
behind Segher's change that caused the wrong .machine.
Consider this source compiled with -mcpu=7400:
void ppcaltivecfunc (void) __attribute__ ((__target__ ("cpu=7400,altivec")));
void ppcaltivecfunc (void)
{
asm ("lvx 0,0,11");
}
void ppc403func (void) __attribute__ ((__target__ ("cpu=403")));
void ppc403func (void)
{
asm ("lvx 0,0,11");
}
Even though the compiler emitted a .machine "403" (or currently the buggy
"ppc"), the assembler won't barf on "lvx 0,0,11" in ppc403func(), because of
the sticky altivec flag.
That's why I suggested more control over the sticky flags via .machine
pseudo-ops, -mdotmachine-realcpu-resets-sticky or similar. That way the users
of pure assembly sources won't see a change in behaviour, but the compiler
still gains the full control it needs.
And I hope the suggested warning options to help the users of existing sources
with multi-cpu inline asm, since they maybe affected when the compiler switches
the assembler to a different mode by default.
[Bug target/101393] PowerPC32 inline assembly broken by commit 2d94f7dea9c73ef3c116a0ddc722724578a860fe
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101393 --- Comment #11 from Alan Modra --- Preserving certain -m gas options goes back to this patch: https://sourceware.org/pipermail/binutils/2008-January/054935.html Given the number of ppc micros around with differing functional units, it is quite reasonable that we have assembly options to say "this base cpu" plus "this extra functionality". Whether you think it was wise to allow those "extra functionality" options to be specified before the "base cpu" option is another matter, but it has been that way for a long time. I'm not inclined to change that since it would very likely break some projects, and I happen to think that it is entirely reasonable to expect that "-maltivec -mppc64" for example is the same as "-mppc64 -maltivec". In any case sticky options are a side issue here. The real issue is that emitted .machine is wrong. Note that I'm not vetoing assembler changes. It might be a good idea to reset all sticky options on processing any .machine directive for example, and I'm happy with the idea of -mno-vsx etc. options for the assembler.
[Bug target/101393] PowerPC32 inline assembly broken by commit 2d94f7dea9c73ef3c116a0ddc722724578a860fe
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101393 --- Comment #10 from Segher Boessenkool --- (In reply to Franz Sirl from comment #9) > (In reply to Segher Boessenkool from comment #8) > > I don't think it is a good idea to add workaround upon workaround to avoid > > some of the not-so-useful behaviours of -many. Instead, we should just > > not use -many? > > As I understand it -many is just one variation of the general problem with > the sticky flags. If we remove -many from the assembler, there are still > other sticky flags like -mvsx. Turning of any sticky flag is currently not > supported by the assembler AFAICS. So for example it's impossible to switch > from a VSX supporting assembler mode to an assembler mode without VSX via > the .machine pseudo-op. Or did I misread the the assembler source? So "sticky" is some internal GAS concept? This isn't documented in the manuals. This means that its behaviour can likely be changed without too much trouble; that would be quite preferable, it's not such a hot idea to have to change what GCC does to something ever more complex, if we could just change GAS instead. Maybe the assembler can adopt -mno-vsx etc., with the same semantics as that has in GCC?
[Bug target/101393] PowerPC32 inline assembly broken by commit 2d94f7dea9c73ef3c116a0ddc722724578a860fe
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101393 --- Comment #9 from Franz Sirl --- (In reply to Segher Boessenkool from comment #8) > I don't think it is a good idea to add workaround upon workaround to avoid > some of the not-so-useful behaviours of -many. Instead, we should just > not use -many? As I understand it -many is just one variation of the general problem with the sticky flags. If we remove -many from the assembler, there are still other sticky flags like -mvsx. Turning of any sticky flag is currently not supported by the assembler AFAICS. So for example it's impossible to switch from a VSX supporting assembler mode to an assembler mode without VSX via the .machine pseudo-op. Or did I misread the the assembler source?
[Bug target/101393] PowerPC32 inline assembly broken by commit 2d94f7dea9c73ef3c116a0ddc722724578a860fe
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101393 Segher Boessenkool changed: What|Removed |Added CC||amodra at gcc dot gnu.org --- Comment #8 from Segher Boessenkool --- (In reply to Franz Sirl from comment #7) > Created attachment 51189 [details] > More complete trial patch to overhaul ASM_CPU_SPEC > > This patch should be more complete now. It does the following basic things: If it does a few things, it should be a few patches, not one. > - Only pass -many to the assembler when -massembler-any is given Interesting idea! I wonder how well that works, how many programs will need to use that new flag (for their inline assembler code). > - Unify the non-AIX (for now) asm_cpu spec settings into rs6000-cpus.def. > Currently this uses a "static inline" function because I developed on GCC10, > on GCC11 and later using a constexpr would be much nicer. > > Now, the other stuff to reliably handle -many would need some gas support, > where I likely need some help, because I've no expertise there. > > First, I suggest to introduce 2 assembler warning options: > > -wany-duplicates: Warn if the mnemonic to assemble has duplicates because > of -many, this should limit the warnings to the most difficult cases. In the > patch it's automatically enabled when -massembler-any is used. > -wany-strict: Warn if any mnemonic to assemble is only fulfilled because of > -many. You'll have to discuss this on binut...@sourceware.org . > And also some additional/changed options for .machine: > > .machine "resetsticky": Reset the sticky flags (eg. VSX, AltiVec, ANY) > .machine "pushsticky": Push CPU+sticky settings, reset the sticky flags > .machine "push": Change to push CPU+sticky, keep the sticky flags > .machine "pop": Change to pop CPU+sticky Why would you want this concept? It is mixing .machine selection with other things. > The attached patch tries to make use of such a TBD change to gas. > > Alternatively, gas could be changed to have -madditive-sticky and/or > '.machine "additive-sticky"' to make any '.machine "realcpu"' reset the > sticky flags and they have to be re-added again afterwards if needed. Maybe > this push/pop-less solution would be even easier to handle in the compiler? > > What do you think? I hope I addressed most of your concerns, suggestions > welcome. I don't think it is a good idea to add workaround upon workaround to avoid some of the not-so-useful behaviours of -many. Instead, we should just not use -many? [ Cc: Alan, for the binutils side of things ]
[Bug target/101393] PowerPC32 inline assembly broken by commit 2d94f7dea9c73ef3c116a0ddc722724578a860fe
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101393 Franz Sirl changed: What|Removed |Added Attachment #51164|0 |1 is obsolete|| --- Comment #7 from Franz Sirl --- Created attachment 51189 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51189=edit More complete trial patch to overhaul ASM_CPU_SPEC This patch should be more complete now. It does the following basic things: - Only pass -many to the assembler when -massembler-any is given - Unify the non-AIX (for now) asm_cpu spec settings into rs6000-cpus.def. Currently this uses a "static inline" function because I developed on GCC10, on GCC11 and later using a constexpr would be much nicer. Now, the other stuff to reliably handle -many would need some gas support, where I likely need some help, because I've no expertise there. First, I suggest to introduce 2 assembler warning options: -wany-duplicates: Warn if the mnemonic to assemble has duplicates because of -many, this should limit the warnings to the most difficult cases. In the patch it's automatically enabled when -massembler-any is used. -wany-strict: Warn if any mnemonic to assemble is only fulfilled because of -many. And also some additional/changed options for .machine: .machine "resetsticky": Reset the sticky flags (eg. VSX, AltiVec, ANY) .machine "pushsticky": Push CPU+sticky settings, reset the sticky flags .machine "push": Change to push CPU+sticky, keep the sticky flags .machine "pop": Change to pop CPU+sticky The attached patch tries to make use of such a TBD change to gas. Alternatively, gas could be changed to have -madditive-sticky and/or '.machine "additive-sticky"' to make any '.machine "realcpu"' reset the sticky flags and they have to be re-added again afterwards if needed. Maybe this push/pop-less solution would be even easier to handle in the compiler? What do you think? I hope I addressed most of your concerns, suggestions welcome.
[Bug target/101393] PowerPC32 inline assembly broken by commit 2d94f7dea9c73ef3c116a0ddc722724578a860fe
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101393
--- Comment #6 from Segher Boessenkool ---
(In reply to Franz Sirl from comment #4)
> How about something along this patch? It's not fully done (no good idea
> about SPEC stuff like "mcpu=7400: -mppc %{!mvsx:%{!maltivec:-maltivec}};"
> yet), but you should get the idea.
What does it mean if you have NULL for the new field? Does that work, even?
If it means there is code in a .c too handle it there, we should use that
*always*, not just for a few entries, since it simplifies things.
> In the the end most of ASM_CPU_SPEC could be removed and also the
> duplication between ASM_CPU_SPEC and driver-rs6000.c could go away.
Maybe? I'd like to see a design for it. Historically, tweaking this stuff
has been very error-prone.
[Bug target/101393] PowerPC32 inline assembly broken by commit 2d94f7dea9c73ef3c116a0ddc722724578a860fe
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101393 --- Comment #5 from Segher Boessenkool --- (In reply to Franz Sirl from comment #3) > (In reply to Segher Boessenkool from comment #1) > > The -many is problematic, that is the whole point of this. As in this > > example: on different subtargets there are different machine code > > translations for the same mnemonic! > > But the way gas works there are never 2 active translations for the same > mnemonic, or? Of course. But it means that the meaning of the assembler code depends on the command line flags, unless a .machine statement is given (or equivalent command line flags). Since the compiler always depends on one particular meaning, this won't fly. > Actually, since -many is sticky and always (except when CHECKING_P?? or AIX) > part of ASM_CPU_SPEC, the ".machine ppc" doesn't really help since -many is > also active for the .machine pseudo-op. We want to get rid of -many if at all possible. This isn't possible currently mostly because of inline asm, but at least we can try too not depend on its (very problematic) effects. > Maybe you meant to also remove -many from ASM_CPU_SPEC? In that case we > would have seen at least an assembler error, since there is no dcread for > -mppc. Yeah, we would like to, but then people who *want* to use an insn not supported by the currently selected cpu have to adjust their inline asm code. It isn't clear to me where to warn for this (to nudge such users).
[Bug target/101393] PowerPC32 inline assembly broken by commit 2d94f7dea9c73ef3c116a0ddc722724578a860fe
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101393
--- Comment #4 from Franz Sirl ---
Created attachment 51164
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51164=edit
Half-baken trial patch
How about something along this patch? It's not fully done (no good idea about
SPEC stuff like "mcpu=7400: -mppc %{!mvsx:%{!maltivec:-maltivec}};" yet), but
you should get the idea.
In the the end most of ASM_CPU_SPEC could be removed and also the duplication
between ASM_CPU_SPEC and driver-rs6000.c could go away.
[Bug target/101393] PowerPC32 inline assembly broken by commit 2d94f7dea9c73ef3c116a0ddc722724578a860fe
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101393 --- Comment #3 from Franz Sirl --- (In reply to Segher Boessenkool from comment #1) > The -many is problematic, that is the whole point of this. As in this > example: on different subtargets there are different machine code > translations for the same mnemonic! But the way gas works there are never 2 active translations for the same mnemonic, or? For example, for "-m403 -many" or "-many -m403" first the 403 mnemonics are added to the hash. Then, for -many, all the mnemonics are added but duplicates are simply skipped. Actually, since -many is sticky and always (except when CHECKING_P?? or AIX) part of ASM_CPU_SPEC, the ".machine ppc" doesn't really help since -many is also active for the .machine pseudo-op. Maybe you meant to also remove -many from ASM_CPU_SPEC? In that case we would have seen at least an assembler error, since there is no dcread for -mppc.
[Bug target/101393] PowerPC32 inline assembly broken by commit 2d94f7dea9c73ef3c116a0ddc722724578a860fe
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101393 --- Comment #2 from Segher Boessenkool --- A good solution would add all those historical targets to rs6000_machine_from_flags.
[Bug target/101393] PowerPC32 inline assembly broken by commit 2d94f7dea9c73ef3c116a0ddc722724578a860fe
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101393 Segher Boessenkool changed: What|Removed |Added CC||segher at gcc dot gnu.org --- Comment #1 from Segher Boessenkool --- The -many is problematic, that is the whole point of this. As in this example: on different subtargets there are different machine code translations for the same mnemonic!
