Re: [PATCH] v2 PR102024 - IBM Z: Add psabi diagnostics
Andreas Krebbel wrote: >gcc/ChangeLog: >PR target/102024 >* config/s390/s390-protos.h (s390_function_arg_vector): Remove >prototype. >* config/s390/s390.cc (s390_single_field_struct_p): New function. >(s390_function_arg_vector): Invoke s390_single_field_struct_p. >(s390_function_arg_float): Likewise. This looks good to me. Bye, Ulrich
Re: [PATCH] s390: Fix up -Wpsabi diagnostcs + analysis [PR94704]
On Tue, Apr 28, 2020 at 02:13:02PM +0200, Jakub Jelinek wrote:
> struct X { };
> struct Y { int : 0; };
> struct Z { int : 0; Y y; };
> struct U : public X { X q; };
> struct A { double a; };
> struct B : public X { double a; };
> struct C : public Y { double a; };
> struct D : public Z { double a; };
> struct E : public U { double a; };
This is only testing the [[no_unique_address]] attribute in the
most-derived class, but I believe the attribute also affects
members in the base class. Is this understanding correct?
Specifically, if we were to add two more tests to the above:
struct X2 { X x; };
struct X3 { [[no_unique_address]] X x; };
struct B2 : public X2 { double a; };
struct B3 : public X3 { double a; };
Then we should see that B2 does *not* count as single-element
struct, but B3 *does*. (That's also what GCC currently does.)
Just trying to get clarity here as I'm about to work on this
for clang ...
Bye,
Ulrich
--
Dr. Ulrich Weigand
GNU/Linux compilers and toolchain
ulrich.weig...@de.ibm.com
[wwwdocs] Re: [PATCH v2] Fix -ffast-math flags handling inconsistencies
On Sat, Nov 21, 2020 at 01:57:32PM -0600, Segher Boessenkool wrote: > On Fri, Nov 20, 2020 at 09:33:48PM -0700, Jeff Law wrote: > > On 2/11/20 11:43 AM, Ulrich Weigand wrote: > > > 1. If a component flag of -ffast-math (or -funsafe-math-optimizations) > > >is explicitly set (or reset) on the command line, this should override > > >any implicit change due to -f(no-)fast-math, no matter in which order > > >the flags come on the command line. This change affects all flags. > > > > > > 2. Any component flag modified from its default by -ffast-math should > > >be reset to the default by -fno-fast-math. This was previously > > >not done for the following flags: > > > -fcx-limited-range > > > -fexcess-precision= > > > > > > 3. Once -ffinite-math-only is true, the -f(no-)signaling-nans flag has > > >no meaning (if we have no NaNs at all, it does not matter whether > > >there is a difference between quiet and signaling NaNs). Therefore, > > >it does not make sense for -ffast-math to imply -fno-signaling-nans. > > >This is also a documentation change. > > > > > > 4. -ffast-math is documented to imply -fno-rounding-math, however the > > >latter setting is the default anyway; therefore it does not make > > >sense to try to modify it from its default setting. > > > > > > 5. The __FAST_MATH__ preprocessor macro should be defined if and only > > >if all the component flags of -ffast-math are set to the value that > > >is documented as the effect of -ffast-math. The following flags > > >were currently *not* so tested: > > > -fcx-limited-range > > > -fassociative-math > > > -freciprocal-math > > > -frounding-math > > >(Note that we should still *test* for -fno-rounding-math here even > > >though it is not set as per 4. -ffast-math -frounding-math should > > >not set the __FAST_MATH__ macro.) > > >This is also a documentation change. > > > It appears this was dropped on the floor.? It looks reasonable to me.? > > Please retest and commit.? Thanks! I've now retested on s390x-ibm-linux and committed to mainline. Thanks for the review! > It all makes sense, and is a nice improvement :-) But please mention it > in the release notes? No doubt people did use non-sensical flag > combinations, and they will be affected. Thanks! Here's a proposed patch to update the gcc-11 changes.hmtl. OK to commit? Bye, Ulrich diff --git a/htdocs/gcc-11/changes.html b/htdocs/gcc-11/changes.html index 46a6a37..c0f896a 100644 --- a/htdocs/gcc-11/changes.html +++ b/htdocs/gcc-11/changes.html @@ -58,6 +58,29 @@ a work-in-progress. is deprecated and will be removed in a future release. It should be possible to use --enable-cheaders=c_global (the default) with no change in behaviour. + + Some inconsistencies in handling combinations of -ffast-math, + -fno-fast-math, -funsafe-math-optimizations, + -fno-unsafe-math-optimizations, and their component flags + have been fixed. This might change the behavior of the compiler when + invoked with certain combinations of such command line options. + The behavior is now consistently: + + If a component flag of -ffast-math or + -funsafe-math-optimizations is explicitly set or reset + on the command line, this will override any implicit change, no matter + in which order the flags come on the command line. + Any component flag (which is not explicity set or reset on the command + line) that was modified from its default by -ffast-math or + -funsafe-math-optimizations is always reset to its default + by a subsequent -fno-fast-math or + -fno-unsafe-math-optimizations. + -ffast-math no longer implicitly changes + -fsignaling-math. + The __FAST_MATH__ preprocessor macro is set if and + only if all component flags of -ffast-math are set + to the value documented as the effect of -ffast-math. + -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain ulrich.weig...@de.ibm.com
Re: [PATCH v2] Fix -ffast-math flags handling inconsistencies
On Fri, Nov 20, 2020 at 09:33:48PM -0700, Jeff Law wrote: > > * doc/invoke.texi (-ffast-math): Remove mention of -fno-signaling-nans. > > Clarify conditions when __FAST_MATH__ preprocessor macro is defined. > > > > * opts.c (common_handle_option): Pass OPTS_SET to set_fast_math_flags > > and set_unsafe_math_optimizations_flags. > > (set_fast_math_flags): Add OPTS_SET argument, and use it to avoid > > setting flags already explicitly set on the command line. In the !set > > case, also reset x_flag_cx_limited_range and x_flag_excess_precision. > > Never reset x_flag_signaling_nans or x_flag_rounding_math. > > (set_unsafe_math_optimizations_flags): Add OPTS_SET argument, and use > > it to avoid setting flags already explicitly set on the command line. > > (fast_math_flags_set_p): Also test x_flag_cx_limited_range, > > x_flag_associative_math, x_flag_reciprocal_math, and > > x_flag_rounding_math. > It appears this was dropped on the floor. It looks reasonable to me. > Please retest and commit. Thanks! This did handle flag_excess_precision incorrectly, causing in particular https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97970 I've reverted for now and will post a modified patch later. Sorry for the breakage! At a first glance, it appears the problem is that -fexcess-precision=fast is already the default, so it does not make sense for -fno-fast-math to "reset" this back to default. Probably the best is for fast-math to simply not touch excess precision at all: - if it is set explicitly on the command line, fast-math should not affect it in any case; - if it is not set explicitly on the command line, the default either with fast-math or no-fast-math is excess-precision=fast, so again it should not be touched. I'll still need to look into the language-specific handling of the excess precision setting to make sure this works for all languages. Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain ulrich.weig...@de.ibm.com
Re: [PATCH v2 01/31] PR target/58901: reload: Handle SUBREG of MEM with a mode-dependent address
On Tue, Nov 24, 2020 at 06:19:41AM +, Maciej W. Rozycki wrote: > NB I find the reindentation resulting in `push_reload' awful, just as I > do either version of the massive logical expression involved. Perhaps we > could factor these out into `static inline' functions sometime, and then > have them split into individual returns within? I'm generally hesitant to do a lot of changes to the reload code base at this stage. The strategy rather is to move all remaining targets over to LRA and then simply delete reload :-) Given that you're modernizing the vax target, I'm wondering if you shouldn't rather go all the way and move it over to LRA as well, then you wouldn't be affected by any remaining reload deficiencies. (The major hurdle so far was that LRA doesn't support CC0, but it looks like you're removing that anyway ...) > + && (strict_low > + || (subreg_lowpart_p (in) > + && (CONSTANT_P (SUBREG_REG (in)) > + || GET_CODE (SUBREG_REG (in)) == PLUS > + || strict_low If we do this, then that "|| strict_low" clause is now redundant. > + && REGNO (SUBREG_REG (in)) >= FIRST_PSEUDO_REGISTER > + && reg_equiv_mem (REGNO (SUBREG_REG (in))) > + && (mode_dependent_address_p > + (XEXP (reg_equiv_mem (REGNO (SUBREG_REG (in))), 0), > +MEM_ADDR_SPACE (reg_equiv_mem (REGNO (SUBREG_REG (in) I guess this is not incorrect, but I'm wondering if it would be *complete* -- there are other cases where reload replaces a pseudo with a MEM even where reg_equiv_mem is null. That said, if it fixes the test suite errors you're seeing, it would probably be OK to go with just this minimal change -- unless we can just move to LRA as mentioned above. Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain ulrich.weig...@de.ibm.com
Re: [PATCH, rs6000][PR gdb/27525] displaced stepping across addpcis/lnia.
On Tue, Mar 16, 2021 at 05:31:03PM -0500, will schmidt wrote: > This addresses PR gdb/27525. The lnia and other variations > of the addpcis instruction write the value of the NIA into a target register. > If we are single-stepping across a breakpoint, the instruction is executed > from a displaced location, and thusly the written value of the PC/NIA > will be incorrect. The changes here will measure the displacement > offset, and adjust the target register value to compensate. > > This is written in a way that I believe will make it easier to > update to handle prefixed (8 byte) instructions in a future patch. This looks good to me functionally, but I'm not sure it really makes much sense to extract code into those new routines -- *all* of the ppc_displaced_step_fixup routine is about handling instructions that read the PC, like the branches do. I'd prefer if the new instructions were simply added to the existing switch alongside the branches. > + displaced_offset = from - to ; /* FIXME - By inspection, it appears > the displaced instruction > + is at a lower address. Is this > always true? */ No, it could be either way. But it shouldn't really matter since you just need to apply the same displaced offset to the target, whether the offset is positive or negative. Again, you should just do it the same way it is already done by existing code that handles branches. Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain ulrich.weig...@de.ibm.com
Re: [PATCH] gdb-power10-single-step
On Thu, Mar 25, 2021 at 12:21:42PM -0500, will schmidt wrote:
> On Wed, 2021-03-10 at 18:50 +0100, Ulrich Weigand wrote:
> > Will Schmidt wrote:
> >
> > > This is a patch written by Alan Modra. With his permission
> > > I'm submitting this for review and helping get this upstream.
> > >
> > > Powerpc / Power10 ISA 3.1 adds prefixed instructions, which
> > > are 8 bytes in length. This is in contrast to powerpc previously
> > > always having 4 byte instruction length. This patch implements
> > > changes to allow GDB to better detect prefixed instructions, and
> > > handle single stepping across the 8 byte instructions.
> >
> > There's a few issues I see here:
>
> I've dug in a bit more,.. have a few questions related to the patch and
> the comments here. I've got a refreshed version of this patch in my
> queue, with a nit or two that i'm still trying to understand and
> squash before I post it.
>
> >
> > - The patch now *forces* software single-stepping for all 8-byte
> > instructions. I'm not sure why this is necessary; I thought
> > that hardware single-stepping was supported for 8-byte instructions
> > as well? That would certainly be preferable.
>
>
> Does software single-stepping go hand-in-hand with executing the
> instructions from a displaced location?
Yes. Hardware single-step executes the instruction where it is.
Software single-step needs to replace the subsequent instruction
with a breakpoint, and in order to be able to do that without
unduly affecting simultaneous execution of that code in other
threads, this is not done in place, but in a copy in a displaced
location.
> I only see one clear return-if-prefixed change in the patch, so I am
> assuming the above refers to the patch chunk seen as :
> > @@ -1081,6 +1090,10 @@ ppc_deal_with_atomic_sequence (struct regcache
> > *regcache)
> >const int atomic_sequence_length = 16; /* Instruction sequence length.
> > */
> >int bc_insn_count = 0; /* Conditional branch instruction count. */
> >
> > + /* Power10 prefix instructions are two words in length. */
> > + if ((insn & OP_MASK) == 1 << 26)
> > +return { pc + 8 };
Yes, this is what I was refering to. By returning a PC value here,
common code is instructed to always perform a software single-step.
This should not be necessary.
> I've got a local change to eliminate that return. Per the poking I've
> done so far, none of the prefix instructions I've run against so far
> allow us past the is_load_and_reserve instruction check.
> > if (!IS_LOAD_AND_RESERVE_INSN (insn))
> > return {};
>
> statement, so not a significant code flow behavior change.
Yes, if you just remove the three lines above, code will fall
through to here and return an empty sequence, causing the
common code to use hardware single-step.
> > - However, the inner loop of ppc_deal_with_atomic_sequence should
> > probably be updated to correctly skip 8-byte instructions; e.g.
> > to avoid mistakenly recognizing the second word of an 8-byte
> > instructions for a branch or store conditional. (Also, the
> > count of up to "16 instructions" is wrong if 8-byte instructions
> > are not handled specifically.)
>
> I've got a local change to inspect the instruction and determine if it
> is prefixed, so I think i've got this handled. I'm generally assuming
> we will never start halfway through an 8-byte prefixed instruction.
Yes, you can assume the incoming PC value is a valid PC at the start
of the current instruction.
Bye,
Ulrich
--
Dr. Ulrich Weigand
GNU/Linux compilers and toolchain
ulrich.weig...@de.ibm.com
