[Mesa-dev] [Mesa-stable] [PATCH v2] llvmpipe: disable vsx in ppc due to LLVM PPC bug

[Roland Scheidegger]

Am 17.11.2015 um 21:27 schrieb Oded Gabbay:
> On Tue, Nov 17, 2015 at 9:40 PM, Jan Vesely <jan.vesely at rutgers.edu> wrote:
>>
>>
>> On Tue, Nov 17, 2015 at 12:37 PM, Oded Gabbay <oded.gabbay at gmail.com> wrote:
>>>
>>> On Tue, Nov 17, 2015 at 6:15 PM, Emil Velikov <emil.l.velikov at gmail.com>
>>> wrote:
>>>> On 17 November 2015 at 16:02, Oded Gabbay <oded.gabbay at gmail.com> wrote:
>>>>> This patch makes sure that if we use altivec (VMX) instructions, we
>>>>> don't
>>>>> use VSX instructions as well, as this cause piglit tests to fail
>>>>>
>>>>> For more details, see: https://llvm.org/bugs/show_bug.cgi?id=25503#c7
>>>>>
>>>>> With this patch, ppc64le reaches parity with x86-64 as far as piglit
>>>>> test
>>>>> suite is concerned.
>>>>>
>>>>> v2:
>>>>> - Added check that we have at least LLVM 3.4
>>>>> - Added the LLVM bug URL as a comment in the code
>>>>>
>>>>> Signed-off-by: Oded Gabbay <oded.gabbay at gmail.com>
>>>>> Cc: "11.0" <mesa-stable at lists.freedesktop.org>
>>>>> ---
>>>>>  src/gallium/auxiliary/gallivm/lp_bld_misc.cpp | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
>>>>> b/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
>>>>> index 7bda118..152593a 100644
>>>>> --- a/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
>>>>> +++ b/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
>>>>> @@ -536,6 +536,10 @@
>>>>> lp_build_create_jit_compiler_for_module(LLVMExecutionEngineRef *OutJIT,
>>>>>
>>>>>  #if defined(PIPE_ARCH_PPC)
>>>>>     MAttrs.push_back(util_cpu_caps.has_altivec ? "+altivec" :
>>>>> "-altivec");
>>>>> +#if HAVE_LLVM >= 0x0304
>>>>> +   /* See LLVM bug https://llvm.org/bugs/show_bug.cgi?id=25503#c7 */
>>>>> +   MAttrs.push_back("-vsx");
>>>> The commit message does not reflect what the patch actually does. I
>>>> cannot object against the patch in any way, although the two should be
>>>> in sync imho.
>>>>
>>>> Base of a very quick look at the llvm bug, I'm leaning that the commit
>>>> msg is correct and the patch is off ?
>>>>
>>>> Thanks
>>>> Emil
>>>
>>> Hmm, I'm not sure I understand what you mean.
>>>
>>> The commit message says: "This patch makes sure that if we use altivec
>>> (VMX) instructions, we don't
>>> use VSX instructions as well, as this cause piglit tests to fail"
>>>
>>> And the patch itself disables the VSX attribute in LLVM backend in
>>> case we use Altivec (VMX) - meaning that no VSX instructions will be
>>> generated alongside Altivec instructions.
>>>
>>> So unless I completely misunderstood something, the commit message and
>>> the patch match.
>>
>>
>> sorry to meddle. the commit message suggests that you only want to disable
>> vsx if altivec is enabled. However, the patch adds -vsx unconditionally. Do
>> you want to ever have "-altivec -vsx"?
>>
>> Jan
> 
> There is no option, AFAIK, that altivec support is missing, but vsx
> support exists. So, if we write "-altivec", then "-vsx" has no
> meaning, cause there isn't vsx support anyway.
> However, just to make it logically correct, I will send another
> version that only disables vsx if altivec is enabled
> 
>
My guess is you could just adjust the comment instead. After all maybe
it's really vsx which causes the bug, not just together with altivec.
Albeit I guess vsx is more of an extension to altivec, so it might not
even be possible, but in any case switching it off always looks like a
good idea to me.

Roland