Re: [PATCH]: BZ #14648: Define HAS_FMA with bit_FMA_Usable

["Carlos O'Donell" <carlos at systemhalted dot org>]

On Tue, Oct 2, 2012 at 8:03 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Oct 2, 2012 at 12:12 AM, Andreas Jaeger <aj@suse.com> wrote:
>> On 10/01/2012 08:55 PM, H.J. Lu wrote:
>>>
>>> On Mon, Oct 1, 2012 at 11:01 AM, Andreas Jaeger <aj@suse.com> wrote:
>>>>
>>>> On 10/01/2012 05:13 PM, H.J. Lu wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> This patch properly detects FMA support and defines HAS_FMA with
>>>>> bit_FMA_Usable.  Tested on FMA machine.  OK for trunk and all applicable
>>>>> branches?
>>>>
>>>>
>>>>
>>>> Please explain the reason for the change better:
>>>
>>>
>>> According to AVX spec:
>>>
>>> http://software.intel.com/sites/default/files/319433-014.pdf
>>>
>>> to detect FMA support, we should do
>>>
>>> INT supports_fma()
>>> {
>>> ; result in eax
>>> mov eax, 1
>>> cpuid
>>> and ecx, 018001000H
>>> cmp ecx, 018001000H; check OSXSAVE, AVX, FMA feature flags
>>> jne not_supported
>>> ; processor supports AVX,FMA instructions and XGETBV is enabled by OS
>>> mov ecx, 0; specify 0 for XFEATURE_ENABLED_MASK register
>>> XGETBV; result in EDX:EAX
>>> and eax, 06H
>>> cmp eax, 06H; check OS has enabled both XMM and YMM state support
>>> jne not_supported
>>> mov eax, 1
>>> jmp done
>>> NOT_SUPPORTED:
>>> mov eax, 0
>>> done:
>>> }
>>>
>>>> Do you only need this to implement 14649? Why do you need to backport
>>>> this?
>>>
>>>
>>> It is a real bug.
>>>
>>>> Do we use HAS_FMA anywhere - and what is the bug if this does not go in?
>>>>
>>>
>>> The bug is similar to
>>>
>>> http://sourceware.org/bugzilla/show_bug.cgi?id=13583
>>> http://sourceware.org/bugzilla/show_bug.cgi?id=14059
>>>
>>> If not fixed, program may crash on FMA machines.
>>
>>
>> HJ, you need to explain better the reasons for your changes.
>> I would have expected something like:
>>  We currently enable FMA if cpuid says it's available but we should only
>> enable it if the OS support XMM/YMM state support etc - like we do with AVX
>> already.
>>
>> Is that correct? If yes, then we should fix the issue - and your fix is ok
>
> That is correct.
>
>> for head. Carlos needs to approve it for the other branches,
>>
>
> Carlos, is this OK for other branches?

Sorry, I will have high latency for the next week or so since Mentor
is doing releases right now.

Yes, please check this in for 2.16, and 2.15 if you do the additional
validation for that branch.

I reviewed the patch and I also reviewed the Intel and AMD docs and I
agree that you can't use FMA without also checking OSXSAVE and making
sure the YMM state support is there.

I don't know why I didn't fix this when I fixed AVX and FMA4 support.

Thanks for fixing this.

Cheers,
Carlos.