Re: [Patch ARM] big-endian support for Neon vext tests

[Richard Earnshaw <rearnsha at arm dot com>]

On 17/09/12 16:50, Christophe Lyon wrote:
> On 17 September 2012 17:21, Richard Earnshaw <rearnsha@arm.com> wrote:
>> On 17/09/12 16:13, Christophe Lyon wrote:
>>> On 17 September 2012 14:56, Richard Earnshaw <rearnsha@arm.com> wrote:
>>>> On 05/09/12 23:14, Christophe Lyon wrote:
>>>>> Hello,
>>>>>
>>>>> Although the recent optimization I have committed to use Neon vext
>>>>> instruction for suitable builtin_shuffle calls does not support
>>>>> big-endian yet, I have written a patch to the existing testcases such
>>>>> they now support big-endian mode.
>>>>>
>>>>> I think it's worth improving these tests since writing the right masks
>>>>> for big-endian (such that the program computes the same results as in
>>>>> little-endian) is not always straightforward.
>>>>>
>>>>> In particular:
>>>>> * I have added some comments in a few tests were it took me a while to
>>>>> find the right mask.
>>>>> * In the case of the test which is executed, I had to force the
>>>>> noinline attribute on the helper functions, otherwise the computed
>>>>> results are wrong in big-endian. It is probably an overkill workaround
>>>>> but it works :-)
>>>>>   I am going to file a bugzilla for this problem.
>>>>>
>>>>> I have checked that replacing calls to builtin_shuffle by the expected
>>>>> Neon vext variant produces the expected results in big-endian mode,
>>>>> and I arranged the big-endian masks to get the same results.
>>>>>
>>>>> Christophe.=
>>>>>
>>>>>
>>>>> neon-vext-big-endian-tests.patch
>>>>>
>>>>>
>>>>> N ÂnârÂÂÃÃ)emÃhÃyhià Âw^âÂÃ
>>>>>
>>>>
>>>>
>>>> I'm not sure about this.  Looking at the documentation in the manual for
>>>> builtin_suffle makes no mention of the results/behaviour being endian
>>>> dependent, which makes me wonder why this test needs to be.
>>>>
>>>> R.
>>>
>>>
>>> Indeed, but I had to modify the mask value in order to get the same
>>> results in big and little-endian.
>>>
>>> If the mask should be the same (it would be much more confortable for
>>> the developers indeed), then GCC needs to be changed/fixed.
>>>
>>
>> That's what I'm trying to establish.  I suspect that there is a bug in
>> GCC for all big-endian code here.
>>
>> What happens for a test of uint8x8_t?
>>
> 
> Well, in my sample testcase in little-endian, I used mask = {2, 3, 4,
> 5, 6, 7, 8, 9}, which can be optimized into vext #2.
> 
> In big-endian mode, explicitly forcing use of vext #2 leads to the
> right result, but to achieve it using builtin_shuffle, I had to change
> the mask into {14, 15, 0, 1, 2, 3, 4, 5}.
> 
> I did read the thread starting at
> http://gcc.gnu.org/ml/gcc-patches/2011-10/msg01133.html and the
> threads it references, and I must admit that I got a bit confused :-)
> 
> IMHO, it's currently impossible for a GCC user to write code using
> vector initializers that would be portable on big and little endian
> targets. It's too much of a headache....
> 
> It was also a purpose of this patch: have someone react if it looked
> inappropriate.
> 
> Thanks for the review,
> 
> Christophe.
> 

I think for big-endian, __builtin_shuffle needs to expand to (for 64-bit
vectors)

	vrev64.<size> mask
	vext

and for 128-bit vectors

	vrev64.<size> mask
	vswap  mask<top-dword>, mask<low-dword>
	vext ...

Obviously, if you've got a literal you can simplify the operations down
to something that doesn't need the extra instructions.

R.