On 10/19/16 12:44, Christophe Lyon wrote:
> On 19 October 2016 at 10:34, Kyrill Tkachov <kyrylo.tkac...@foss.arm.com>
> wrote:
>>
>> On 19/10/16 07:55, Christophe Lyon wrote:
>>>
>>> On 18 October 2016 at 17:35, Christophe Lyon <christophe.l...@linaro.org>
>>> wrote:
>>>>
>>>> On 18 October 2016 at 16:45, Bernd Edlinger <bernd.edlin...@hotmail.de>
>>>> wrote:
>>>>>
>>>>> On 10/18/16 10:36, Christophe Lyon wrote:
>>>>>>
>>>>>> I am seeing a lot of regressions since this patch was committed:
>>>>>>
>>>>>> http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/241273/report-build-info.html
>>>>>>
>>>>>> (you can click on "REGRESSED" to see the list of regressions, "sum"
>>>>>> and "log" to download
>>>>>> the corresponding .sum/.log)
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Christophe
>>>>>>
>>>>> Oh, sorry, I have completely missed that.
>>>>> Unfortunately I have tested one of the good combinations.
>>>>>
>>>>> I have not considered the case that in and out could be
>>>>> the same register. But that happens here.
>>>>>
>>>>>
>>>>> This should solve it.
>>>>>
>>>>> Can you give it a try?
>>>>>
>>>> Sure, I have started the validations, it will take a few hours.
>>>>
>>> It looks OK, thanks.
>>
>>
>> Ok. Thanks for testing Christophe.
>> Sorry for not catching it in review.
>> Kyrill
>>
>
> Since it was painful to check that this 2nd patch fixed all the regressions
> observed in all the configurations, I ran another validation with
> the 2 patches combined, on top of r241272 to check the effect of the two
> over the previous reference.
>
> It turns out there is still a failure:
> http://people.linaro.org/~christophe.lyon/cross-validation/gcc-test-patches/241272-r241273-combined/report-build-info.html
> gcc.c-torture/execute/pr34971.c -O0 execution test
> now fails on arm-none-linux-gnueabihf when using thumb mode, expect
> when targeting cortex-a5.
>
Thanks Christophe, I looked in the test case,
and I think that is a pre-existing issue.
I can make the test case fail before my patch:
struct foo
{
unsigned long long b:40;
} x;
extern void abort (void);
void test1(unsigned long long res)
{
/* Build a rotate expression on a 40 bit argument. */
if ((x.b<<9) + (x.b>>31) != res)
abort ();
}
int main()
{
x.b = 0x0100000001;
test1(0x0000000202);
x.b = 0x0100000000;
test1(0x0000000002);
return 0;
}
fails with -mcpu=cortex-a9 -mthumb -mfpu=neon-fp16 -O0
even before my patch.
before reload we have this insn:
(insn 19 18 20 2 (parallel [
(set (reg:DI 113 [ _4 ])
(lshiftrt:DI (reg:DI 112 [ _3 ])
(const_int 31 [0x1f])))
(clobber (scratch:SI))
(clobber (scratch:SI))
(clobber (scratch:DI))
(clobber (reg:CC 100 cc))
]) "pr34971.c":11 1232 {lshrdi3_neon}
which is replaced to this insn:
(insn 19 18 20 2 (parallel [
(set (reg:DI 1 r1 [orig:113 _4 ] [113])
(lshiftrt:DI (reg:DI 0 r0 [orig:112 _3 ] [112])
(const_int 31 [0x1f])))
(clobber (scratch:SI))
(clobber (scratch:SI))
(clobber (scratch:DI))
(clobber (reg:CC 100 cc))
]) "pr34971.c":11 1232 {lshrdi3_neon}
(nil))
And now that is not supposed to happen, when this code
is executed for shifts by a constant less than 32:
emit_insn (SET (out_down, LSHIFT (code, in_down, amount)));
emit_insn (SET (out_down,
ORR (REV_LSHIFT (code, in_up, reverse_amount),
out_down)));
emit_insn (SET (out_up, SHIFT (code, in_up, amount)));
out_down may not be the same hard register than in_up for this to work.
I opened a new BZ for this:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78041
I am not sure if this is a LRA issue or if it can be handled in the
shift expansion.
Is the second patch good for trunk as it is?
Thanks
Bernd.
