[PATCH try 2] [i386,testsuite,libgcc] Fix build breakage on Mac and test FAILS on Solaris caused by PR82196 patch

Daniel Santos daniel.santos@pobox.com
Sat Sep 23 08:57:00 GMT 2017


On 09/22/2017 06:50 AM, Uros Bizjak wrote:
> On Fri, Sep 22, 2017 at 1:27 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Fri, Sep 22, 2017 at 12:28 PM, Daniel Santos <daniel.santos@pobox.com> wrote:
>>> On 09/22/2017 03:28 AM, Rainer Orth wrote:
>>>> Hi Daniel,
>>>>
>>>>> On 09/22/2017 02:18 AM, Rainer Orth wrote:
>>>>>> Hi Daniel,
>>>>>>
>>>>>>> On 09/21/2017 05:18 PM, Daniel Santos wrote:
>>>>>>>> So libgcc doesn't use a config.in. :(
>>>>>>> Scratch that, I forgot that we're using gcc/config.in via auto-host.h.
>>>>>>> So I only have to add this to gcc/configure.ac and it will be available
>>>>>>> for my libgcc header -- this is what I used to sniff out support for the
>>>>>>> .hidden directive.
>>>>>> Please don't go that route: it's totally the wrong direction.  There's
>>>>>> work going on to further decouple libgcc from gcc-private headers and
>>>>>> configure results.  libgcc already has its own configure tests for
>>>>>> assembler features, and its own config.in.  What's wrong with adapting
>>>>>> libitm's avx test in libitm/acinclude.m4 (LIBITM_CHECK_AS_AVX) for
>>>>>> libgcc?  Should be trivial...
>>>>>>
>>>>>>     Rainer
>>>>>>
>>>>> Oops, I just saw your email after submitting my other patch.  Yes, I am
>>>>> mistaken about config.in, sorry about that.  I didn't see a config.h
>>>>> file, but examining further it looks like it outputs to auto-target.h.
>>>>> Also, I was looking for some HAVE_AS* macros, but they are named
>>>>> differently.
>>>> Right: though some are for assembler features, the macros are named
>>>> differently.
>>>>
>>>>> I had previously included gcc's auto-host.h since it was in the include
>>>>> path in order to use HAVE_AS_HIDDEN, so in order to decouple this I'll
>>>> HAVE_GAS_HIDDEN actually ;-)
>>>>
>>>>> need to add that check into libgcc/configure.ac as well.  Again,
>>>>> shouldn't be that much code.  Sound sane to you?
>>>> You could do that, but it was already used before your patches, so
>>>> please separate it from the current issue if you go down that route.
>>>> libgcc is still full of cleanup possibilities :-)
>>>>
>>>>       Rainer
>>> OK, so I'm just adding HAVE_AS_AVX mostly as-is from libitm (we don't
>>> have $target_cpu so I'm using $target).  I do have minor concerns about
>>> how this test will work on a cross-build -- I'm not an autotools expert
>>> and I don't understand which assembler it will invoke, but the results
>>> of the test failing only means we use .byte instead of the real
>>> mnemonic, so it really shouldn't be a problem.
>>>
>>> I've got tests started again, so presuming that *this* one passes, is it
>>> OK for the trunk?
>>>
>>> gcc/testsuite:
>>>         * gcc.target/i386/pr82196-1.c: Simplify so that it doesn't break
>>>         on Solaris or with -mno-omit-frame-pointer.
>> No need to explain the change in the ChangeLog. Just say "(b): Remove
>> volatile asm."
>>
>>>         * gcc.target/i386/pr82196-2.c: Likewise.
>>>
>>> libgcc:
>>>         * configure.ac: Add check for HAVE_AS_AVX.
>>>         * config.in: Regenerate.
>>>         * configure: Likewise.
>>>         * config/i386/i386-asm.h: Include auto-target.h from libgcc.
>>>         (SSE_SAVE, SSE_RESTORE): Sniff HAVE_AS_AVX and directly emit raw
>>>         .byte code when assembler doesn't support avx, correct
>>>         out-of-date comments.
>> (SSE_SAVE, SSE_RESTORE): Emit .byte sequence for !HAVE_AS_AVX.
>> Correct out-of-date comments.
>>
>>>  gcc/testsuite/gcc.target/i386/pr82196-1.c |  5 ++-
>>>  gcc/testsuite/gcc.target/i386/pr82196-2.c |  5 ++-
>>>  libgcc/config.in                          |  3 ++
>>>  libgcc/config/i386/i386-asm.h             | 45 ++++++++++++++++++++++-----
>>>  libgcc/configure                          | 39 +++++++++++++++++++++++
>>>  libgcc/configure.ac                       | 16 ++++++++++
>>>  6 files changed, 100 insertions(+), 13 deletions(-)
>>
>>  #ifdef MS2SYSV_STUB_AVX
>>  # define MS2SYSV_STUB_PREFIX __avx_
>> -# define MOVAPS vmovaps
>> +# ifdef HAVE_AS_AVX
>> +#  define MOVAPS vmovaps
>> +# endif
>>
>> This is unecessarily complex. Please define MOVAPS unconditionaly, and ...
> Please disregard the above... It is OK, since the code also handles sse.

Also, a little bit of the complexity is due to the header being intended
for use other than for these stubs.  So I only define SSE_SAVE and
SSE_RESTORE if (in essence) one of MS2SYSV_STUB_AVX or MS2SYSV_STUB_SSE
are defined prior to the #include.  I try to balance thinking ahead and
genericity with trying not to make things overly complex for an
imaginary, would-be future user -- I may not always succeed at striking
that balance though. :)

>
>> +#  define BYTE .byte
>> +#  define SSE_SAVE    \
>> + BYTE 0xc5, 0x78, 0x29, 0x78, 0xd0; /* vmovaps %xmm15,-0x30(%rax) */ \
>>
>> Is there a reason for BYTE definition? Every known assembler supports
>> .byte directive.
> + BYTE 0xc5, 0xf8, 0x28, 0x76, 0x60; /* vmovaps  0x60(%rsi),%xmm6  */
> +# endif /* MOVAPS */
>  #endif /* defined (MS2SYSV_STUB_ISA) && defined (MOVAPS) */
>  #endif /* I386_ASM_H */
>
> Please update the #endif comment.
>
> Uros.

Thanks, more uglies that I won't have to correct later!

Thanks,
Daniel



More information about the Gcc-patches mailing list