This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [Aarch64] PR target/83009: Relax strict address checking for store pair lanes
On 30/05/18 16:11, Sudakshina Das wrote:
> Hi Andre
>
> On 30/05/18 11:41, Sudakshina Das wrote:
>> On 19/05/18 04:12, James Greenhalgh wrote:
>>>
>>>> On 8 May 2018, at 02:58, Andre Vieira (lists)
>>>> <Andre.SimoesDiasVieira@arm.com> wrote:
>>>>
>>>> Hi Richard,
>>>>> On 07/05/18 11:14, Richard Sandiford wrote:
>>>>> "Andre Vieira (lists)" <Andre.SimoesDiasVieira@arm.com> writes:
>>>>>> Hi,
>>>>>>
>>>>>> See below a patch to address PR 83009.
>>>>>>
>>>>>> Tested with aarch64-linux-gnu bootstrap and regtests for c, c++
>>>>>> and fortran.
>>>>>> Ran the adjusted testcase for -mabi=ilp32.
>>>>>>
>>>>>> Is this OK for gcc-9?
>>>>>>
>>>>>> Cheers,
>>>>>> Andre
>>>>>>
>>>>>> PR target/83009: Relax strict address checking for store pair lanes
>>>>>>
>>>>>> The operand constraint for the memory address of store/load pair
>>>>>> lanes
>>>>>> was enforcing strictly hardware registers be allowed as memory
>>>>>> addresses. We want to relax that such that these patterns can be
>>>>>> used
>>>>>> by combine. During register allocation the register constraint will
>>>>>> enforce the correct register is chosen.
>>>>>
>>>>> Nice spot.
>>>>>
>>>>>> diff --git a/gcc/config/aarch64/predicates.md
>>>>>> b/gcc/config/aarch64/predicates.md
>>>>>> index
>>>>>> 5d41d4350402b2a9e5941f160c6ab6f933bfff90..f29bc8e74f0070589014ac87fd22a95723ba9be8
>>>>>> 100644
>>>>>> --- a/gcc/config/aarch64/predicates.md
>>>>>> +++ b/gcc/config/aarch64/predicates.md
>>>>>> @@ -222,7 +222,7 @@
>>>>>> ;; as a 128-bit vec_concat.
>>>>>> (define_predicate "aarch64_mem_pair_lanes_operand"
>>>>>> (and (match_code "mem")
>>>>>> - (match_test "aarch64_legitimate_address_p (DFmode, XEXP
>>>>>> (op, 0), 1,
>>>>>> + (match_test "aarch64_legitimate_address_p (DFmode, XEXP
>>>>>> (op, 0), 0,
>>>>>> ADDR_QUERY_LDP_STP)")))
>>>>>>
>>>>>> (define_predicate "aarch64_prefetch_operand"
>>>>>
>>>>> Very minor, but it'd be good to change it to a real bool parameter
>>>>> at the same time, for consistency with aarch64_mem_pair_operand.
>>>>> (Patch LGTM otherwise FWIW.)
>>>>>
>>>> Good shout! Thank you. Attached new version.
>>>
>>> I’m happy to take Richard’s review here. The patch looks good to me.
>>>
>>> Ok for trunk,
>>> This commit has caused a miscompare failure in Spec2006 434.zeusmp
>> on aarch64-none-linux-gnu
>>
>> Running Benchmarks
>> Running 434.zeusmp ref base bld default
>> /home/suddas01/spec2006/src/spec2006/bin/specinvoke -d
>> /home/suddas01/spec2006/src/spec2006/benchspec/CPU2006/434.zeusmp/run/run_base_ref_bld.0000
>> -e speccmds.err -o speccmds.stdout -f speccmds.cmd -C -q
>> /home/suddas01/spec2006/src/spec2006/bin/specinvoke -E -d
>> /home/suddas01/spec2006/src/spec2006/benchspec/CPU2006/434.zeusmp/run/run_base_ref_bld.0000
>> -c 1 -e compare.err -o compare.stdout -f compare.cmd
>>
>> *** Miscompare of tsl000aa; for details see
>>
>> /home/suddas01/spec2006/src/spec2006/benchspec/CPU2006/434.zeusmp/run/run_base_ref_bld.0000/tsl000aa.mis
>>
>> Invalid run; unable to continue.
>>
>
> I tried with changing the 'false' parameter in your patch to
> reload_completed as suggested by Wilco.
>
> diff --git a/gcc/config/aarch64/predicates.md
> b/gcc/config/aarch64/predicates.md
> index 4814b93..2d3123d 100644
> --- a/gcc/config/aarch64/predicates.md
> +++ b/gcc/config/aarch64/predicates.md
> @@ -226,7 +226,7 @@
> ;; as a 128-bit vec_concat.
> (define_predicate "aarch64_mem_pair_lanes_operand"
> (and (match_code "mem")
> - (match_test "aarch64_legitimate_address_p (DFmode, XEXP (op, 0),
> false,
> + (match_test "aarch64_legitimate_address_p (DFmode, XEXP (op, 0),
> reload_completed,
> ADDR_QUERY_LDP_STP)")))
>
> (define_predicate "aarch64_prefetch_operand"
>
> But it is still failing on the said benchmark. It is possible that there
> is something more hidden that came out because of this patch rather
> than the patch itself.
>
> Would it be possible to temporarily revert this patch for now and look
> at it separately?
>
Yeah I have reverted my patch and am looking into this. I believe the
problem arises from a stp with register + immediate offset being printed
with offset 8 when it should have been 16.
I'll go try out some changes and should come back with a new patch soon.
> Sudi
>
>>
>>> Thanks,
>>> James
>>>
>>> (Trying a mobile mail client, apologies if this bounces)
>>>
>>
>