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

[Christophe Lyon <christophe dot lyon at linaro dot org>]

On 13 September 2012 19:07, Mike Stump <mikestump@comcast.net> wrote:
> On Sep 13, 2012, at 2:45 AM, Christophe Lyon <christophe.lyon@linaro.org> wrote:
>> Ping?
>> http://gcc.gnu.org/ml/gcc-patches/2012-09/msg00330.html
>
> So, two things I thought I'd ask about:
>
>> +/* __attribute__ ((noinline)) is currently required, otherwise the
>> +   generated code computes wrong results in big-endian.  */
>
> and:
>
>> +#ifdef __ARMEL__
>> +  uint64x2_t __mask1 = {1, 0};
>> +#else
>>    uint64x2_t __mask1 = {1, 0};
>> +#endif
>
>
>>> * 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 think that for developing the patches noinline was fine, we are confident there aren't any more bugs,
> but, for checkin, I think it is better to leave the test case as is, and let it fail until the PR you filed is fixed.
>  We usually don't put hack arounds for code-gen compiler bugs into the testsuite just to make them
> pass…  :-)

OK. So I will remove the noinline stuff, and update the bugzilla entry
with the name of this testcase.
Should I really leave the test as FAILED rather then XFAIL?

> The second (occurs more than once) just looks odd.  I thought I'd mention it, not sure what my preference is.
Well... it was just for consistency will all the other tests as in
general the mask vectors are different in big and little endian, and
to expose the fact that I hadn't forgotten to write the big-endian
variant :-)

I can remove these occurrences and add a comment instead.


Thanks for your review,

Christophe.