Re: [PATCH mips] Remove fp64 multilibs from mips-mti-* targets.
OK.
-eric
On Fri, Aug 8, 2014 at 1:07 PM, Steve Ellcey wrote:
> Here is another MIPS patch. This removes the fp64 multilib from the
> mips-mti-* targets. With the new fpxx we no longer want special fp64
> multilibs in the mti targets. Since it doesn't affect any other targets
> hopefully there is no objection to checking this in.
>
> Tested on mips-mti-linux-gnu and mips-mti-elf targets.
>
> OK to checkin?
>
> Steve Ellcey
> sell...@mips.com
>
>
>
> 2014-08-08 Steve Ellcey
>
> * config/mips/t-mti-elf (MULTILIB_OPTIONS): Remove fp64 multilib.
> (MULTILIB_DIRNAMES): Ditto.
> * config/mips/t-mti-elf (MULTILIB_OPTIONS): Ditto.
> * config/mips/t-mti-elf (MULTILIB_EXCEPTIONS): Ditto.
> * config/mips/t-mti-linux (MULTILIB_OPTIONS): Ditto.
> * config/mips/t-mti-linux (MULTILIB_DIRNAMES): Ditto.
> * config/mips/t-mti-linux (MULTILIB_EXCEPTIONS): Ditto.
> * config/mips/mti-linux.h (SYSROOT_SUFFIX_SPEC): Ditto.
>
> diff --git a/gcc/config/mips/mti-linux.h b/gcc/config/mips/mti-linux.h
> index db9896b..318e981 100644
> --- a/gcc/config/mips/mti-linux.h
> +++ b/gcc/config/mips/mti-linux.h
> @@ -20,7 +20,7 @@ along with GCC; see the file COPYING3. If not see
> /* This target is a multilib target, specify the sysroot paths. */
> #undef SYSROOT_SUFFIX_SPEC
> #define SYSROOT_SUFFIX_SPEC \
> -
> "%{mips32:/mips32}%{mips64:/mips64}%{mips64r2:/mips64r2}%{mips16:/mips16}%{mmicromips:/micromips}%{mabi=64:/64}%{mel|EL:/el}%{msoft-float:/sof}%{mfp64:/fp64}%{mnan=2008:/nan2008}"
> +
> "%{mips32:/mips32}%{mips64:/mips64}%{mips64r2:/mips64r2}%{mips16:/mips16}%{mmicromips:/micromips}%{mabi=64:/64}%{mel|EL:/el}%{msoft-float:/sof}%{mnan=2008:/nan2008}"
>
> #undef DRIVER_SELF_SPECS
> #define DRIVER_SELF_SPECS \
> diff --git a/gcc/config/mips/t-mti-elf b/gcc/config/mips/t-mti-elf
> index cd0a967..487a015 100644
> --- a/gcc/config/mips/t-mti-elf
> +++ b/gcc/config/mips/t-mti-elf
> @@ -19,8 +19,8 @@
> # The default build is mips32r2, hard-float big-endian. Add mips32,
> # soft-float, and little-endian variations.
>
> -MULTILIB_OPTIONS = mips32/mips64/mips64r2 mips16/mmicromips mabi=64 EL
> msoft-float/mfp64 mnan=2008
> -MULTILIB_DIRNAMES = mips32 mips64 mips64r2 mips16 micromips 64 el sof fp64
> nan2008
> +MULTILIB_OPTIONS = mips32/mips64/mips64r2 mips16/mmicromips mabi=64 EL
> msoft-float mnan=2008
> +MULTILIB_DIRNAMES = mips32 mips64 mips64r2 mips16 micromips 64 el sof nan2008
> MULTILIB_MATCHES = EL=mel EB=meb mips32r2=mips32r3 mips32r2=mips32r5
> mips64r2=mips64r3 mips64r2=mips64r5
>
> # The 64 bit ABI is not supported on the mips32 architecture.
> @@ -43,8 +43,3 @@ MULTILIB_EXCEPTIONS += *mmicromips/mabi=64*
>
> # We do not want nan2008 libraries for soft-float.
> MULTILIB_EXCEPTIONS += *msoft-float*/*mnan=2008*
> -
> -# -mfp64 libraries are only built for mips32r2 and not in mips16 mode.
> -MULTILIB_EXCEPTIONS += *mips32/*mfp64*
> -MULTILIB_EXCEPTIONS += *mips64*/*mfp64*
> -MULTILIB_EXCEPTIONS += *mips16*/*mfp64*
> diff --git a/gcc/config/mips/t-mti-linux b/gcc/config/mips/t-mti-linux
> index cd0a967..487a015 100644
> --- a/gcc/config/mips/t-mti-linux
> +++ b/gcc/config/mips/t-mti-linux
> @@ -19,8 +19,8 @@
> # The default build is mips32r2, hard-float big-endian. Add mips32,
> # soft-float, and little-endian variations.
>
> -MULTILIB_OPTIONS = mips32/mips64/mips64r2 mips16/mmicromips mabi=64 EL
> msoft-float/mfp64 mnan=2008
> -MULTILIB_DIRNAMES = mips32 mips64 mips64r2 mips16 micromips 64 el sof fp64
> nan2008
> +MULTILIB_OPTIONS = mips32/mips64/mips64r2 mips16/mmicromips mabi=64 EL
> msoft-float mnan=2008
> +MULTILIB_DIRNAMES = mips32 mips64 mips64r2 mips16 micromips 64 el sof nan2008
> MULTILIB_MATCHES = EL=mel EB=meb mips32r2=mips32r3 mips32r2=mips32r5
> mips64r2=mips64r3 mips64r2=mips64r5
>
> # The 64 bit ABI is not supported on the mips32 architecture.
> @@ -43,8 +43,3 @@ MULTILIB_EXCEPTIONS += *mmicromips/mabi=64*
>
> # We do not want nan2008 libraries for soft-float.
> MULTILIB_EXCEPTIONS += *msoft-float*/*mnan=2008*
> -
> -# -mfp64 libraries are only built for mips32r2 and not in mips16 mode.
> -MULTILIB_EXCEPTIONS += *mips32/*mfp64*
> -MULTILIB_EXCEPTIONS += *mips64*/*mfp64*
> -MULTILIB_EXCEPTIONS += *mips16*/*mfp64*
Re: [PATCH mips] Pass -msoft-float/-mhard-float flags to GAS
On Sat, Aug 9, 2014 at 12:00 PM, Matthew Fortune
wrote:
> Moore, Catherine writes:
>> > -Original Message-
>> > From: Steve Ellcey [mailto:sell...@mips.com]
>> > Sent: Friday, August 08, 2014 3:42 PM
>> > To: Moore, Catherine; matthew.fort...@imgtec.com; echri...@gmail.com;
>> >
>> > 2014-08-08 Steve Ellcey
>> >
>> > * config/mips/mips.h (ASM_SPEC): Pass float options to assembler.
>> >
>> > diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h index
>> > 8d7a09f..9a15287 100644
>> > --- a/gcc/config/mips/mips.h
>> > +++ b/gcc/config/mips/mips.h
>> > @@ -1187,6 +1187,8 @@ struct mips_cpu_info { %{mshared} %{mno-
>> > shared} \ %{msym32} %{mno-sym32} \ %{mtune=*} \
>> > +%{mhard-float} %{msoft-float} \
>> > +%{msingle-float} %{mdouble-float} \
>> > %(subtarget_asm_spec)"
>> >
>> > /* Extra switches sometimes passed to the linker. */
>> >
>>
>> Hi Steve,
>> The patch itself looks okay, but perhaps a question for Matthew.
>> Does the fact that the assembler requires -msoft-float even if .set
>> softfloat is present in the .s file deliberate behavior?
>
> The assembler requires -msoft-float if .gnu_attribute 4,3 is given. I.e. the
> overall module options must match the ABI which has been specified. .set
> directives can still be used to override the 'current' options and be
> inconsistent with the overall module and/or .gnu_attribute setting.
>
>> I don't have a problem with passing along the *float* options to gas, but
>> would hope that the .set options were honored as well.
>
> Yes they should be.
>
>> My short test indicated that the .set *float* options were being ignored if
>> the correct command line option wasn't present.
>
> I'm not certain what you are describing here. Could you confirm with an
> example
> just in case something is not working as expected?
>
I don't have one offhand, but in skimming the binutils patch I don't
recall seeing anything that tested this combination. May have missed
it though.
That said, the patch looks ok and if you'd like to add some tests for
.set and the command line options to binutils that'd be great as well.
-eric
Re: [PATCH mips] Do not compile mips16.S in soft-float mode
>> >> -#ifdef __mips_micromips >> +#if defined(__mips_micromips) || defined(__mips_soft_float) >>/* DO NOTHING */ >> #else >> Mind adding a comment here explaining why we don't want to do anything for soft float (and micromips)? OK otherwise. Thanks! -eric
Re: [PATCH mips] Do not compile mips16.S in soft-float mode
Thanks! -eric On Tue, Aug 12, 2014 at 8:33 AM, Steve Ellcey wrote: > On Tue, 2014-08-12 at 00:07 -0700, Eric Christopher wrote: >> >> >> >> -#ifdef __mips_micromips >> >> +#if defined(__mips_micromips) || defined(__mips_soft_float) >> >>/* DO NOTHING */ >> >> #else >> >> >> >> Mind adding a comment here explaining why we don't want to do anything >> for soft float (and micromips)? >> >> OK otherwise. >> >> Thanks! >> >> -eric > > OK, I added this comment to the checkin: > > #if defined(__mips_micromips) || defined(__mips_soft_float) > /* Do nothing because this code is only needed when linking > against mips16 hard-float objects. Neither micromips code > nor soft-float code can be linked against mips16 hard-float > objects so we do not need these routines when building libgcc > for those cases. */ > > > Steve Ellcey > sell...@mips.com > >
Re: [PATCH mips] Pass -msoft-float/-mhard-float flags to GAS
On Tue, Aug 12, 2014 at 2:07 AM, Matthew Fortune
wrote:
> Eric Christopher writes:
>> On Sat, Aug 9, 2014 at 12:00 PM, Matthew Fortune
>> wrote:
>> > Moore, Catherine writes:
>> >> > -Original Message-
>> >> > From: Steve Ellcey [mailto:sell...@mips.com]
>> >> > Sent: Friday, August 08, 2014 3:42 PM
>> >> > To: Moore, Catherine; matthew.fort...@imgtec.com; echri...@gmail.com;
>> >> >
>> >> > 2014-08-08 Steve Ellcey
>> >> >
>> >> > * config/mips/mips.h (ASM_SPEC): Pass float options to assembler.
>> >> >
>> >> > diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h index
>> >> > 8d7a09f..9a15287 100644
>> >> > --- a/gcc/config/mips/mips.h
>> >> > +++ b/gcc/config/mips/mips.h
>> >> > @@ -1187,6 +1187,8 @@ struct mips_cpu_info { %{mshared} %{mno-
>> >> > shared} \ %{msym32} %{mno-sym32} \ %{mtune=*} \
>> >> > +%{mhard-float} %{msoft-float} \
>> >> > +%{msingle-float} %{mdouble-float} \
>> >> > %(subtarget_asm_spec)"
>> >> >
>> >> > /* Extra switches sometimes passed to the linker. */
>> >> >
>> >>
>> >> Hi Steve,
>> >> The patch itself looks okay, but perhaps a question for Matthew.
>> >> Does the fact that the assembler requires -msoft-float even if .set
>> >> softfloat is present in the .s file deliberate behavior?
>> >
>> > The assembler requires -msoft-float if .gnu_attribute 4,3 is given. I.e.
>> the
>> > overall module options must match the ABI which has been specified. .set
>> > directives can still be used to override the 'current' options and be
>> > inconsistent with the overall module and/or .gnu_attribute setting.
>> >
>> >> I don't have a problem with passing along the *float* options to gas,
>> but
>> >> would hope that the .set options were honored as well.
>> >
>> > Yes they should be.
>> >
>> >> My short test indicated that the .set *float* options were being ignored
>> if
>> >> the correct command line option wasn't present.
>> >
>> > I'm not certain what you are describing here. Could you confirm with an
>> example
>> > just in case something is not working as expected?
>> >
>>
>> I don't have one offhand, but in skimming the binutils patch I don't
>> recall seeing anything that tested this combination. May have missed
>> it though.
>>
>> That said, the patch looks ok and if you'd like to add some tests for
>> .set and the command line options to binutils that'd be great as well.
>
> What sort of coverage do you think we need here? I did exhaustive coverage
> of anything which can affect the ABI but don't think we need the same for
> command-line options vs .module vs .set.
>
I probably would, but that's a different review thread the the patch
is already in so I'm not going to push it here :)
> There were two pre-existing tests: mips-hard-float-flag and
> mips-double-float-flag which pretty much test these cases I think. Is that
> OK for now until we identify any other areas which need additional
> coverage?
Sure.
-eric
Re: [build, driver] RFC: Support compressed debug sections
>> If it is just to reach compatibility with the debugger, then I’d rather >> either just mandate a certain debugger or autoconf for what the current >> debugger supports. As of late people seem to just break the debugging >> experience with non-updated gdbs and assume that a newer gdb is used. > > You cannot do that: unlike the assembler and linker used, which are > often hardcoded into gcc, the debugger can easily be changed below the > compiler's feet, so to speak. Besides, on several platforms, you have > more than one debugger available (like gdb and dbx, or others), so this > isn't an option. Apart from that, the debugging experience when > e.g. emitting very recent DWARF extensions and trying to use them with a > gdb that doesn't understand them usually leads to some debug info > missing. In this case, emitting compressed debug with a debugger that > cannot read it leads to the debugger claiming (correctly, from its > point of view) that there's no debugging info present. I don't want to > tell users who come complaining `I compiled with -g, but my debugger > tells me there's no debug info present': `look, your debugger lies, it > is present, but it cannot read it'. That's a lot worse than the > DWARF extensions scenario above. > Agreed :) FWIW it's already a gas/assembler option, I'm curious about wanting to expose it via the compiler? > On top of all that, compressed debug is a tradeoff: in some cases it may > be worth it to save space on debug info if disk space is at a premium > for some reason (e.g. for release builds), but in others you want to > compile as fast as possible, but assembling and linking compressed debug > takes more CPU time. Otherwise we could just as well default to -Os, > telling our users it's better for them since it generates faster and > smaller code, not minding the compile time cost and worse debugging > experience. > FWIW I've found in some limited timing that compression is nearly always worth it here at Google - even for compile time given the cost of writing files versus cpu time. Might be worth making it a default at some point in the future and making sure the option is invertible. -eric
Re: [build, driver] RFC: Support compressed debug sections
On Thu, Jun 26, 2014 at 6:32 AM, Rainer Orth wrote: > Eric Christopher writes: > >>>> If it is just to reach compatibility with the debugger, then I’d rather >>>> either just mandate a certain debugger or autoconf for what the current >>>> debugger supports. As of late people seem to just break the debugging >>>> experience with non-updated gdbs and assume that a newer gdb is used. >>> >>> You cannot do that: unlike the assembler and linker used, which are >>> often hardcoded into gcc, the debugger can easily be changed below the >>> compiler's feet, so to speak. Besides, on several platforms, you have >>> more than one debugger available (like gdb and dbx, or others), so this >>> isn't an option. Apart from that, the debugging experience when >>> e.g. emitting very recent DWARF extensions and trying to use them with a >>> gdb that doesn't understand them usually leads to some debug info >>> missing. In this case, emitting compressed debug with a debugger that >>> cannot read it leads to the debugger claiming (correctly, from its >>> point of view) that there's no debugging info present. I don't want to >>> tell users who come complaining `I compiled with -g, but my debugger >>> tells me there's no debug info present': `look, your debugger lies, it >>> is present, but it cannot read it'. That's a lot worse than the >>> DWARF extensions scenario above. >> >> Agreed :) >> >> FWIW it's already a gas/assembler option, I'm curious about wanting to >> expose it via the compiler? > > One reason: ease of use: > > * -gz is far easier to use/type than -Wa,--compress-debug-sections + > -Wl,--compress-debug-sections, and > Very true. Maybe make it a -gcompress-dwarf-sections? > * one common option irrespective of assemblers (the Solaris assembler > will gain eventually gain compressed debug support, too) and linkers > used (Solaris ld requires -z compress-sections=), and even the > Apple assembler might at some point ;-) > The assembler itself does, but as far as I know none of the consumers can deal with it. Right now it supports the same options as gas. >>> On top of all that, compressed debug is a tradeoff: in some cases it may >>> be worth it to save space on debug info if disk space is at a premium >>> for some reason (e.g. for release builds), but in others you want to >>> compile as fast as possible, but assembling and linking compressed debug >>> takes more CPU time. Otherwise we could just as well default to -Os, >>> telling our users it's better for them since it generates faster and >>> smaller code, not minding the compile time cost and worse debugging >>> experience. >> >> FWIW I've found in some limited timing that compression is nearly >> always worth it here at Google - even for compile time given the cost >> of writing files versus cpu time. Might be worth making it a default >> at some point in the future and making sure the option is invertible. > > One might be not so lucky with different/slower CPUs, though. I wonder > how this would affect bootstrap times on my current SPARC systems ;-( > I'd have thought you'd still largely be write bound for compilation. *shrug* > But yes, a configure option to default -gz to on would certainly be > helpful at some point. *nod* -eric
Re: [PATCH][MIPS] Fix ICE in bitmap routines with LRA and inline assembly language
On Thu, Sep 18, 2014 at 1:11 PM, Eric Christopher wrote:
>
>
>
> On Wed, Sep 10, 2014 at 3:39 AM, Matthew Fortune
> wrote:
>>
>> > The patch is ok to commit. I think I made a typo as other analogous
>> > code contains '|='.
>> >
>> > Thanks for fixing this, Robert.
>>
>> The LRA part of this is committed as r215119.
>>
>> Eric: Is the test OK to commit for MIPS? I suggest removing the
>> dg-skip-if as the test is for successful compilation, not inspecting
>> the generated code.
>>
Agreed. Sorry for the delay.
-eric
>
>>
>> Thanks,
>> Matthew
>>
>> >
>> > >
>> > > 2014-09-09 Robert Suchanek
>> > >
>> > > gcc/testsuite/
>> > > * gcc.target/mips/20140909.c: New test.
>>
>> > > diff --git a/gcc/testsuite/gcc.target/mips/20140909.c
>> > b/gcc/testsuite/gcc.target/mips/20140909.c
>> > > new file mode 100644
>> > > index 000..dbdfb30
>> > > --- /dev/null
>> > > +++ b/gcc/testsuite/gcc.target/mips/20140909.c
>> > > @@ -0,0 +1,20 @@
>> > > +/* { dg-do compile } */
>> > > +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" } { "" } } */
>> > > +
>> > > +NOMIPS16 int NoBarrier_AtomicIncrement(volatile int* ptr, int
>> > increment) {
>> > > + int temp, temp2;
>> > > + __asm__ __volatile__(".set push\n"
>> > > + ".set noreorder\n"
>> > > + "1:\n"
>> > > + "ll %0, 0(%3)\n"
>> > > + "addu %1, %0, %2\n"
>> > > + "sc %1, 0(%3)\n"
>> > > + "beqz %1, 1b\n"
>> > > + "addu %1, %0, %2\n"
>> > > + ".set pop\n"
>> > > + : "=&r" (temp), "=&r" (temp2)
>> > > + : "Ir" (increment), "r" (ptr)
>> > > + : "memory");
>> > > +
>> > > + return temp2;
>> > > +}
>> > >
>>
>
Re: [Patch AArch64] Fix extended register width
On Tue, Sep 30, 2014 at 5:57 AM, Marcus Shawcroft wrote: > On 22 September 2014 19:41, Carrot Wei wrote: >> Hi >> >> The extended register width in add/adds/sub/subs/cmp instructions is >> not always the same as target register, it depends on both target >> register width and extension type. But in current implementation the >> extended register width is always the same as target register. We have >> noticed it can generate following wrong assembler code when compiled >> an internal application, >> >> add x2, x20, x0, sxtw 3 >> >> The correct assembler should be >> >> add x2, x20, w0, sxtw 3 > Hi Marcus, > Hi, > > The assembler deliberately accepts the first form as a programmer > convenience. Given the above example: > I've been doing some reading of the ARM-v8 ARM and the language the ARM uses here for this instruction matches the "shall" and not "should" language it uses in other locations: "Is the 32-bit name of the second general-purpose source register, encoded in the "Rm" field." This seems to say that a conforming assembler should error on a non-32bit named register here. As I said, same sort of verbiage used elsewhere for shall, in "should" cases the ARM is very careful to spell it out. Now if we want to change the ARM philosophy here I'm not opposed, but I think we'd want some more explicit documentation about how/where things should be more relaxed versus a bunch of "this is convenient to accept here" stuff. That kind of thing has a tendency to end up in some pretty fun context sensitive parsing madness. Thoughts? -eric > AARCH64 GAS x.s page 1 > > >1 82CE20ABaddsx2, x20, x0, sxtw 3 >2 0004 82CE20ABaddsx2, x20, w0, sxtw 3 > > Note both forms are correctly assembled. The GAS implementation > contains code at (or near) tc-aarch64.c:5461 that specifically catches > the former. > > ... therefore I see no need to change the behaviour of gcc. > > Cheers > /Marcus
Re: [Patch AArch64] Fix extended register width
On Wed, Oct 1, 2014 at 1:42 AM, Richard Earnshaw wrote: > On 30/09/14 21:30, Eric Christopher wrote: >> On Tue, Sep 30, 2014 at 5:57 AM, Marcus Shawcroft >> wrote: >>> On 22 September 2014 19:41, Carrot Wei wrote: >>>> Hi >>>> >>>> The extended register width in add/adds/sub/subs/cmp instructions is >>>> not always the same as target register, it depends on both target >>>> register width and extension type. But in current implementation the >>>> extended register width is always the same as target register. We have >>>> noticed it can generate following wrong assembler code when compiled >>>> an internal application, >>>> >>>> add x2, x20, x0, sxtw 3 >>>> >>>> The correct assembler should be >>>> >>>> add x2, x20, w0, sxtw 3 >>> >> >> Hi Marcus, >> >>> Hi, >>> >>> The assembler deliberately accepts the first form as a programmer >>> convenience. Given the above example: >>> >> >> I've been doing some reading of the ARM-v8 ARM and the language the >> ARM uses here for this instruction matches the "shall" and not >> "should" language it uses in other locations: >> >> "Is the 32-bit name of the second general-purpose source register, >> encoded in the "Rm" field." >> >> This seems to say that a conforming assembler should error on a >> non-32bit named register here. As I said, same sort of verbiage used >> elsewhere for shall, in "should" cases the ARM is very careful to >> spell it out. >> >> Now if we want to change the ARM philosophy here I'm not opposed, but >> I think we'd want some more explicit documentation about how/where >> things should be more relaxed versus a bunch of "this is convenient to >> accept here" stuff. That kind of thing has a tendency to end up in >> some pretty fun context sensitive parsing madness. >> > > Agreed. We're already working on it... > To be clear here I think the current language is exactly what it should be in this case and that the explicit w register requirement is a good thing for the assembly language. -eric
[PATCH] Use CHECKSUM_ macros and ULEB128 checksum for DIE tag
Hi Cary,
This patch changes the ODR checker to use the CHECKSUM_ macros and
instead of depending on size of int to use the ULEB128 of the tag
(similar to the deep hash) to compute the values.
Thoughts?
-eric
2013-07-22 Eric Christopher
* dwarf2out.c (die_odr_checksum): New function to use
CHECKSUM_ macros and ULEB128 for DIE tag.
(generate_type_signature): Use.
Index: gcc/dwarf2out.c
===
--- gcc/dwarf2out.c (revision 198904)
+++ gcc/dwarf2out.c (working copy)
@@ -6097,6 +6097,13 @@
CHECKSUM_ULEB128 (0);
}
+static void
+die_odr_checksum (dw_die_ref die, md5_ctx *ctx)
+{
+ CHECKSUM_ULEB128(die->die_tag);
+ CHECKSUM_STRING(get_AT_string(die, DW_AT_name));
+}
+
#undef CHECKSUM
#undef CHECKSUM_STRING
#undef CHECKSUM_ATTR
@@ -6128,7 +6135,6 @@
/* First, compute a signature for just the type name (and its surrounding
context, if any. This is stored in the type unit DIE for link-time
ODR (one-definition rule) checking. */
-
if (is_cxx() && name != NULL)
{
md5_init_ctx (&ctx);
@@ -6137,8 +6143,8 @@
if (parent != NULL)
checksum_die_context (parent, &ctx);
- md5_process_bytes (&die->die_tag, sizeof (die->die_tag), &ctx);
- md5_process_bytes (name, strlen (name) + 1, &ctx);
+ /* Checksum the current DIE. */
+ die_odr_checksum(die, &ctx);
md5_finish_ctx (&ctx, checksum);
add_AT_data8 (type_node->root_die, DW_AT_GNU_odr_signature,
&checksum[8]);
Re: [PATCH] Use CHECKSUM_ macros and ULEB128 checksum for DIE tag
On Mon, Jul 22, 2013 at 1:30 PM, Cary Coutant wrote: > - md5_process_bytes (&die->die_tag, sizeof (die->die_tag), &ctx); > - md5_process_bytes (name, strlen (name) + 1, &ctx); > + /* Checksum the current DIE. */ > + die_odr_checksum(die, &ctx); > > Since we've already gone to the trouble of getting the name of this > DIE, it seems wasteful to have die_odr_checksum call get_AT_string > again. Why not just pass name as a parameter? > Was mostly being cute and assuming it'd get removed. I'll just pass both of them down into the function for now. If we decide later that we want to change what's hashed we can change the function. >> I guess you should add a function comment, plus spaces before ( everywhere >> (and while you're at it, you can change also the is_cxx() ). Will leave the >> rest to Cary. > > Agree with the function comment and the spaces. Oh yeah, thanks. Sorry, it's been a while. > >> 2013-07-22 Eric Christopher >> >> * dwarf2out.c (die_odr_checksum): New function to use >> CHECKSUM_ macros and ULEB128 for DIE tag. >> (generate_type_signature): Use. > > This is OK with those changes. Thanks! > Committed thusly: ghostwheel:~/sources/gcc> svn ci Sendinggcc/ChangeLog Sendinggcc/dwarf2out.c Transmitting file data .. Committed revision 201148. Thanks! -eric
Re: Darwin bootstrap failure following wide int merge (was: we are starting the wide int merge)
On Mon, May 26, 2014 at 1:14 AM, FX wrote: >> > .././../gcc-4.10-20140518/gcc/wide-int.cc:1274:23: error: invalid use of a >> > cast in a inline asm context requiring an l-value: remove the cast or >> > build with -fheinous-gnu-extensions >> > umul_ppmm (val[1], val[0], op1.ulow (), op2.ulow ()); >> > ~~~^ >> >> This is PR 61146. You can get around it by adding -fheinous-gnu-extensions >> to BOOT_CFLAGS. > > This causes GCC bootstrap to fail on Darwin systems (whose system compiler is > clang-based). Since PR 61146 was resolved as INVALID (but I’m not sure it’s > the right call, see below), I’ve filed a separate report for the bootstrap > issue (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61315). > > Regarding PR 61146, I agree with Marc Glisse (comment #3) that the casts in > question look weird and should probably be removed, as was done in GMP. This > code should be cleaned up, and it will fix bootstrap on clang-based target > coincidentally, without adding another kludge. > I think that posting a patch is probably the best bet. Then the various merits of the patch to clean up the code can be argued. As far as some sort of workaround, I'd suggest seeing if there's something else that can be done first. Thanks. -eric
Re: [PATCH, rs6000] Fix aggregate alignment ABI issue
On Wed, Jul 9, 2014 at 11:25 AM, David Edelsohn wrote: > On Wed, Jul 9, 2014 at 12:04 PM, Ulrich Weigand wrote: >> Hello, >> >> last year, Bill added a patch to address PR 57949 by aligning aggregates >> requiring at least 128-bit alignment at a quadword boundary in the >> parameter save area: >> https://gcc.gnu.org/ml/gcc-patches/2013-08/msg00803.html >> >> Unfortunately, to implement this check, Bill's patch used a pre-existing >> piece of code originally used only on Darwin, which uses a "mode == BLKmode" >> check to test for aggregate types. >> >> However, GCC may sometimes choose non-BLKmode modes to represent aggregate >> types. One case the single-element float/vector aggregates; that's OK, >> because those are handled separately by the ABI anyway. >> >> But there are more cases: many structures get some integer mode simply >> because their size happen to be 1, 2, 4, 8, or 16 bytes. The precise >> rules *which* aggregates GCC uses such a mode for are intricate, and >> even differ slightly between types created by the C vs. C++ front-ends. >> >> This normally doesn't matter since the mode used to back an aggregate >> type only matters for internal code generation (basically, whether GCC >> may use a register to hold a local variable of that type, or whether >> they must all go to memory). >> >> Due to this check in rs6000_function_arg_boundary, however, those GCC >> internal details have now leaked into the public ABI. We have thought >> of simply accepting that ABI as the de-facto ABI now and documenting >> it, but that turned out to be too fragile; it is hard to precisely >> describe the mode selection in a way that it can be reliably implemented >> by another (non-GCC based) compiler. >> >> After various off-line discussions, we came to the conclusion that the >> best way is to fix the GCC implementation to actually align *all* aggregate >> types, not just those backed by BLKmode. [ The exception remain single- >> element (or ELFv2 homogeneous) float/vector aggregates, which are handled >> as before: float is doubleword aligned, vector is quadword aligned. ] >> >> This change does break the ABI in certain cases. However, we hope that >> this is acceptable because: >> >> - The change only affects rare cases: passing a struct by value that is >>* not a float/vector special case, and >>* has a size of 1, 2, 4, 8, or 16 bytes, and >>* has an alignment requirement of 16 bytes or more >> [ Not *all* these cases will see change, but all cases that change >> will share these properties. ] >> >> - This aspect of the ABI already changed recently with Bill's patch, >> and the current version hasn't seen very widespread use yet. >> >> Note that patch below only changes the ABI for the AIX/ELFv1 and ELFv2 >> cases; the Darwin ABI (which shared the same problem) is left as-is. >> It's up to the Darwin maintainers whether they prefer to change as well >> or rather keep everything as it has been on Darwin for a long time. >> >> Tested on powerpc64-linux and powerpc64le-linux. >> >> OK for mainline? >> [ The patch should then also go into the 4.8 and 4.9 branches for >> consistency. ] >> >> Bye, >> Ulrich >> >> >> ChangeLog: >> >> * config/rs6000/rs6000.c (rs6000_function_arg_boundary): In the AIX >> and ELFv2 ABI, do not use the "mode == BLKmode" check to test for >> aggregate types. Instead, *all* aggregate types, except for single- >> element or homogeneous float/vector aggregates, are quadword-aligned >> if required by their type alignment. > > Okay. > > I copied the Darwin maintainers and active testers so that they are > explicitly aware of the ABI issue. They can decide if they want to fix > the ABI alignment issue on Darwin. > Thanks David, In general I'd personally prefer to fix the ABI issues, but PPC darwin is beyond EoL by the original company and I don't have any hardware for it myself - in which case I'll leave it up to our more active testers or someone with hardware. (Mike? Have old ppc hardware sitting around?) -eric
Re: [PATCH, rs6000] Fix aggregate alignment ABI issue
On Wed, Jul 9, 2014 at 12:01 PM, Mike Stump wrote: > On Jul 9, 2014, at 11:29 AM, Eric Christopher wrote: >>>> - The change only affects rare cases: passing a struct by value that is >>>> * not a float/vector special case, and >>>> * has a size of 1, 2, 4, 8, or 16 bytes, and >>>> * has an alignment requirement of 16 bytes or more > >>> I copied the Darwin maintainers and active testers so that they are >>> explicitly aware of the ABI issue. They can decide if they want to fix >>> the ABI alignment issue on Darwin. > >> Thanks David, In general I'd personally prefer to fix the ABI issues, >> but PPC darwin is beyond EoL by the original company and I don't have >> any hardware for it myself - in which case I'll leave it up to our >> more active testers or someone with hardware. (Mike? Have old ppc >> hardware sitting around?) > > Well, I think from a safety perspective I think it is ok, as the system > header files and most libraries I would expect not to align to 16 bytes or > more. The problem with this line of thinking, it only takes 1 structure, in > one place that is used often (stat, X11) to completely kill things. Normally > I would do a world build with the change in it, and a fink build with it in > it, and if no changes occur, the change is reasonably safe. I don’t have the > machines/system to do either unfortunately. I had been testing ppc with > emulation, so I can’t do much of that anymore either. > > So, I think I will punt to the users that still have G5 darwin, they know who > they are… I’d say, lets leave it as is, and if they think it is a good idea > as well (that would make it 3 of 3) and can do a test suite run at least with > the change, then I’d approve it. Completely agreed. -eric
Re: Some DWARFv5 proposal prototypes (atomic_type, aligned_type)
On Tue, Jul 15, 2014 at 4:36 AM, Mark Wielaard wrote: > On Mon, 2014-07-14 at 13:58 -0600, Tom Tromey wrote: >> > "Mark" == Mark Wielaard writes: >> Mark> The following two patches are based on the earlier "dwarf2out.c: Pass >> Mark> one cv_quals argument" and "Emit DW_tag_restrict_type" patches I posted >> Mark> earlier. Since there currently is not even an early draft yet for >> DWARFv5, >> Mark> just a collection of proposals that might or might not be adopted these >> Mark> patches are not meant to be integrated just yet. But I would like to >> get >> Mark> feedback on the code and whether the new debug information is useful. >> >> I didn't read the patches I'm afraid. >> However, the new debug information is definitely useful for the gcc/gdb >> integration project. Without it I think the user can end up with >> mysterious failures in their compiled expressions. > > I don't know how far DWARFv5 is at the moment, these proposals haven't > even made it to a draft yet, so we might have to look at turning them > into GNU extensions first then. > Dwarf5 is fairly well locked down at this point. But that doesn't stop us from getting this fairly well defined so we can put it in 6 and it can just be an extension until then. -eric > But first I need some feedback on the dwarf2out.c refactoring for > passing around cv-qualifiers and the addition of the restrict tag > patches on which these DWARFv5 prototypes are based: > > https://gcc.gnu.org/ml/gcc-patches/2014-07/msg00569.html > https://gcc.gnu.org/ml/gcc-patches/2014-07/msg00575.html > > Thanks, > > Mark >
[PATCH, docs] Document -z option
Just to document that it's passed directly on to the linker.
OK? Wording changes?
-eric
2014-07-15 Eric Christopher
* doc/invoke.texi (Link Options): Document -z option.
Index: gcc/doc/invoke.texi
===
--- gcc/doc/invoke.texi (revision 212574)
+++ gcc/doc/invoke.texi (working copy)
@@ -464,7 +464,7 @@
-static-libasan -static-libtsan -static-liblsan -static-libubsan @gol
-shared -shared-libgcc -symbolic @gol
-T @var{script} -Wl,@var{option} -Xlinker @var{option} @gol
--u @var{symbol}}
+-u @var{symbol} -z @var{keyword}}
@item Directory Options
@xref{Directory Options,,Options for Directory Search}.
@@ -10690,6 +10690,12 @@
Pretend the symbol @var{symbol} is undefined, to force linking of
library modules to define it. You can use @option{-u} multiple times with
different symbols to force loading of additional library modules.
+
+@item -z @var{keyword}
+@opindex z
+@option{-z} is passed directly on to the linker along with the keyword
+@var{keyword}. See the section in the documentation of your linker for
+permitted values and their meanings.
@end table
@node Directory Options
Re: [PATCH, docs] Document -z option
Thanks :)
dzur:~/sources/gcc> svn ci
Enter passphrase for key '/usr/local/google/home/echristo/.ssh/id_dsa':
Sendinggcc/ChangeLog
Sendinggcc/doc/invoke.texi
Transmitting file data ..
Committed revision 212575.
-eric
On Tue, Jul 15, 2014 at 2:30 PM, Diego Novillo wrote:
> On Tue, Jul 15, 2014 at 5:23 PM, Eric Christopher wrote:
>> Just to document that it's passed directly on to the linker.
>>
>> OK? Wording changes?
>>
>> -eric
>>
>> 2014-07-15 Eric Christopher
>>
>> * doc/invoke.texi (Link Options): Document -z option.
>>
>> Index: gcc/doc/invoke.texi
>> ===
>> --- gcc/doc/invoke.texi (revision 212574)
>> +++ gcc/doc/invoke.texi (working copy)
>> @@ -464,7 +464,7 @@
>> -static-libasan -static-libtsan -static-liblsan -static-libubsan @gol
>> -shared -shared-libgcc -symbolic @gol
>> -T @var{script} -Wl,@var{option} -Xlinker @var{option} @gol
>> --u @var{symbol}}
>> +-u @var{symbol} -z @var{keyword}}
>>
>> @item Directory Options
>> @xref{Directory Options,,Options for Directory Search}.
>> @@ -10690,6 +10690,12 @@
>> Pretend the symbol @var{symbol} is undefined, to force linking of
>> library modules to define it. You can use @option{-u} multiple times with
>> different symbols to force loading of additional library modules.
>> +
>> +@item -z @var{keyword}
>> +@opindex z
>> +@option{-z} is passed directly on to the linker along with the keyword
>> +@var{keyword}. See the section in the documentation of your linker for
>> +permitted values and their meanings.
>> @end table
>
> Looks fine.
>
>
> Diego.
Re: [PATCH][AArch64] Vectorise bswap[16,32,64]
Testcase weirdness?
for (i < 0; i < N; ++i)
{
arr[i] = i;
expect[i] = __builtin_bswap64 (i);
if (y) /* Avoid vectorisation. */
abort ();
}
i < 0 :)
duplicated in all 3 testcases btw.
-eric
On Tue, Apr 15, 2014 at 4:25 AM, Kyrill Tkachov wrote:
> Hi all,
>
> This patch enables aarch64 to vectorise bswap[16,32,64] operations by using
> the AdvancedSIMD forms of the rev[16,32,64] instructions.
>
> The TARGET_VECTORIZE_BUILTIN_VECTORIZED_FUNCTION hook is extended to return
> the vectorised forms of __builtin_bswap* where possible and vector bswap
> patterns are added.
>
> I've added the tests in vect.exp and a new effective target check
> (vect_bswap) that can be extended for other targets in the future if they
> can also vectorise these operations. Is that ok?
>
> Bootstrapped and tested aarch64-none-linux-gnu.
>
> Ok for trunk?
>
> Thanks,
> Kyrill
>
> 2014-04-15 Kyrylo Tkachov
>
> * config/aarch64/aarch64-builtins.c
> (aarch64_builtin_vectorized_function): Handle BUILT_IN_BSWAP16,
> BUILT_IN_BSWAP32, BUILT_IN_BSWAP64.
> * config/aarch64/aarch64-simd.md (bswap): New pattern.
> * config/aarch64/aarch64-simd-builtins.def: Define vector bswap
> builtins.
> * config/aarch64/iterator.md (VDQHSD): New mode iterator.
> (Vrevsuff): New mode attribute.
>
> 2014-04-15 Kyrylo Tkachov
>
> * lib/target-supports.exp (check_effective_target_vect_bswap): New.
> * gcc.dg/vect/vect-bswap16: New test.
> * gcc.dg/vect/vect-bswap32: Likewise.
> * gcc.dg/vect/vect-bswap64: Likewise.
