Re: [PATCH] Fix ICEs with ss{e,e2,se3} builtins with -mno-mmx (PR target/82483)

["H.J. Lu" <hjl dot tools at gmail dot com>]

On 10/9/17, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Oct 09, 2017 at 09:28:58PM +0200, Uros Bizjak wrote:
>> On Mon, Oct 9, 2017 at 9:16 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > Hi!
>> >
>> > The following builtins have {SSE,SSE2,SSSE3} CPUID, yet they have
>> > MMX register operands (either input or output or both).
>> > As -mno-mmx doesn't imply -mno-sse, we get ICEs on these though,
>> > because
>> > with MMX disabled it is not possible to emit them.
>> >
>> > The following patch cheats and declares these builtins to require
>> > both the MMX and {SSE,SSE2,SSSE3} ISA, making them unavailable in
>> > -mno-mmx
>> > code, but I think it is the right thing to do, when somebody uses
>> > -mno-mmx,
>> > the intent is usually not to have any instructions touching the MMX
>> > register
>> > file and thus needing EMMS etc.
>> >
>> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>>
>> Hm, I remember there was similar patch for PR79565 [1], which seems
>> related. Does your patch also fix all builtins that HJ's patch touch?
>
> Ah, I wasn't aware of that patch.  Will look in detail tomorrow.
>
>> [1] https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00751.html
>
> Just quick analysis, while -mno-mmx doesn't imply -mno-sse (that is
> right), it implies -mno-3dnow and -mno-3dnow implies -mno-3dnowa.
> So all the spots that were changed from OPTION_MASK_ISA_3DNOW
> to OPTION_MASK_ISA_3DNOW | OPTION_MASK_ISA_MMX or from
> OPTION_MASK_ISA_3DNOW_A to OPTION_MASK_ISA_3DNOW_A | OPTION_MASK_ISA_MMX
> were useless, we can do what we did before.
> It is just OPTION_MASK_ISA_SSE or higher with OPTION_MASK_ISA_MMX that
> is needed.
> I have no idea why __builtin_ia32_sfence or __builtin_ia32_movntq were
> changed.
> Nor I understand why any sse.md changes are needed, then the builtins
> aren't available, those insns shouldn't be synthetized out of thin air.
> And the i386.c def_builtin change IMNSHO isn't needed either, it is
> a compile time speed/memory optimization not to declare everything
> unconditionally, and the way it works right now is that any of the enabled
> ISAs enables the builtin, meaning that MMX enabled will mean those
> 35 will be declared, even when perhaps SSE isn't enabled, or for
> SSE when MMX isn't enabled, but that isn't that big deal.
>
>

Please free feel to change it as long as there is no regression.

Thanks.


-- 
H.J.