[PATCH][AArch64][GCC 6] PR target/79041: Correct -mpc-relative-literal-loads logic in aarch64_classify_symbol

[Yvan Roux]

Hi,

On 22 June 2017 at 20:42, Yvan Roux <yvan.roux@linaro.org> wrote:
> Hi all,
>
> On 16 January 2017 at 16:34, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
>>
>> On 13/01/17 16:35, James Greenhalgh wrote:
>>>
>>> On Wed, Jan 11, 2017 at 04:32:45PM +0000, Kyrill Tkachov wrote:
>>>>
>>>> Hi all,
>>>>
>>>> In this PR we generated ADRP/ADD instructions with :lo12: relocations on
>>>> symbols even though -mpc-relative-literal-loads is used. This is due to
>>>> the
>>>> confusing double-negative logic of the nopcrelative_literal_loads aarch64
>>>> variable and its relation to the aarch64_nopcrelative_literal_loads
>>>> global
>>>> variable in the GCC 6 branch.
>>>>
>>>> Wilco fixed this on trunk as part of a larger patch (r237607) and parts
>>>> of
>>>> that patch were backported, but other parts weren't and that patch now
>>>> doesn't apply cleanly to the branch.
>>>
>>> As I commented to Jakub at the time he made the first partial backport,
>>> I'd much rather just see all of Wilco's patch backported. We're not on
>>> the verge of a 6 release now, so please just backport Wilco's patch (as
>>> should have been done all along if this had been correctly identified as
>>> a fix rather than a clean-up).
>>
>>
>> Unfortunately simply backporting the patch does not fix this PR.
>> The aarch64_classify_symbol changes do not have the desired effect
>> and the :lo12: relocations are generated.
>> I'll look into it, but I believe that would require a bigger change than
>> this one-liner.
>
> Here is the backport of Wilco's patch (r237607) along with Kyrill's
> one (r244643, which removed the remaining occurences of
> aarch64_nopcrelative_literal_loads).  To fix the issue the original
> patch has to be modified, to keep aarch64_pcrelative_literal_loads
> test for large models in aarch64_classify_symbol.
>
> On trunk and gcc-7-branch the :lo12: relocations are not generated
> because of Wilco's fix for pr78733 (r243456 and 243486), but my
> understanding is that the bug is still present since compiling
> gcc.target/aarch64/pr78733.c with -mcmodel=large brings back the
> :lo12: relocations (I'll submit a patch to add the test back if my
> understanding is correct).
>
> Cross built and regtested without issue for aarch64-linux-gnu,
> aarch64-none-elf and aarch64_be-none-elf targets
>
> OK for gcc-6-branch ?

Sorry for the fast ping, but since a 6.4 rc1 is planned for tomorrow,
do you think that this fix can be part of it ?

> Thanks
> Yvan
>
> gcc/ChangeLog
> 2017-06-22  Yvan Roux  <yvan.roux@linaroi.org>
>
>         PR target/79041
>         Backport from mainline
>         2016-06-20  Wilco Dijkstra  <wdijkstr@arm.com>
>
>         * config/aarch64/aarch64.opt
>         (mpc-relative-literal-loads): Rename internal option name.
>         * config/aarch64/aarch64.c
>         (aarch64_nopcrelative_literal_loads): Rename to
>         aarch64_pcrelative_literal_loads.
>         (aarch64_expand_mov_immediate): Likewise.
>         (aarch64_secondary_reload): Likewise.
>         (aarch64_can_use_per_function_literal_pools_p): Likewise.
>         (aarch64_classify_symbol): Likewise.
>         (aarch64_override_options_after_change_1): Rename and simplify logic.
>
>         2016-01-19  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>         * config/aarch64/aarch64-protos.h(aarch64_nopcrelative_literal_loads):
>         Delete.
>         * config/aarch64/aarch64.md
>         (aarch64_reload_movcp<GPF_TF:mode><P:mode>): Delete reference to
>         aarch64_nopcrelative_literal_loads.
>         (aarch64_reload_movcp<VALL:mode><P:mode>): Likewise.
>
> gcc/testsuite/ChangeLog
> 2017-06-22  Yvan Roux  <yvan.roux@linaroi.org>
>
>         PR target/79041
>         * gcc.target/aarch64/pr79041.c: New test.