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

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

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?

Thanks.

-- 
H.J.