On Sat, Sep 23, 2017 at 11:03 AM, Daniel Santos <daniel.san...@pobox.com> wrote: > On 09/22/2017 06:50 AM, Uros Bizjak wrote: >> On Fri, Sep 22, 2017 at 1:27 PM, Uros Bizjak <ubiz...@gmail.com> wrote: >>> On Fri, Sep 22, 2017 at 12:28 PM, Daniel Santos <daniel.san...@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!
The patch is OK for mainline, with the above comment fixed. Let's unbreak Darwin build. Thanks, Uros.
