This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[PING][PATCH, ARM] PR71607: New approach to arm_disable_literal_pool
- From: "Andre Vieira (lists)" <Andre dot SimoesDiasVieira at arm dot com>
- To: gcc-patches at gcc dot gnu dot org
- Cc: Ramana Radhakrishnan <Ramana dot Radhakrishnan at arm dot com>
- Date: Mon, 16 Jan 2017 15:25:35 +0000
- Subject: [PING][PATCH, ARM] PR71607: New approach to arm_disable_literal_pool
- Authentication-results: sourceware.org; auth=none
- References: <57F6583E.4030002@arm.com> <CAJA7tRZ5-dTmqdBupM8+j4=yaZuEubSrMZKnnAw=+1pa2J2UbQ@mail.gmail.com> <583D4E22.5050400@arm.com> <58638CBC.9000303@arm.com>
On 28/12/16 09:58, Andre Vieira (lists) wrote:
> On 29/11/16 09:45, Andre Vieira (lists) wrote:
>> On 17/11/16 10:00, Ramana Radhakrishnan wrote:
>>> On Thu, Oct 6, 2016 at 2:57 PM, Andre Vieira (lists)
>>> <Andre.SimoesDiasVieira@arm.com> wrote:
>>>> Hello,
>>>>
>>>> This patch tackles the issue reported in PR71607. This patch takes a
>>>> different approach for disabling the creation of literal pools. Instead
>>>> of disabling the patterns that would normally transform the rtl into
>>>> actual literal pools, it disables the creation of this literal pool rtl
>>>> by making the target hook TARGET_CANNOT_FORCE_CONST_MEM return true if
>>>> arm_disable_literal_pool is true. I added patterns to split floating
>>>> point constants for both SF and DFmode. A pattern to handle the
>>>> addressing of label_refs had to be included as well since all
>>>> "memory_operand" patterns are disabled when
>>>> TARGET_CANNOT_FORCE_CONST_MEM returns true. Also the pattern for
>>>> splitting 32-bit immediates had to be changed, it was not accepting
>>>> unsigned 32-bit unsigned integers with the MSB set. I believe
>>>> const_int_operand expects the mode of the operand to be set to VOIDmode
>>>> and not SImode. I have only changed it in the patterns that were
>>>> affecting this code, though I suggest looking into changing it in the
>>>> rest of the ARM backend.
>>>>
>>>> I added more test cases. No regressions for arm-none-eabi with
>>>> Cortex-M0, Cortex-M3 and Cortex-M7.
>>>>
>>>> Is this OK for trunk?
>>>
>>> Including -mslow-flash-data in your multilib flags ? If no regressions
>>> with that ok .
>>>
>>>
>>> regards
>>> Ramana
>>>
>>>>
>>
>> Hello,
>>
>> I found some new ICE's with the -mslow-flash-data testing so I had to
>> rework this patch. I took the opportunity to rebase it as well.
>>
>> The problem was with the way the old version of the patch handled label
>> references. After some digging I found I wasn't using the right target
>> hook and so I implemented the 'TARGET_USE_BLOCKS_FOR_CONSTANT_P' for
>> ARM. This target hook determines whether a literal pool ends up in an
>> 'object_block' structure. So I reverted the changes made in the old
>> version of the patch to the ARM implementation of the
>> 'TARGET_CANNOT_FORCE_CONST_MEM' hook and rely on
>> 'TARGET_USE_BLOCKS_FOR_CONSTANT_P' instead. This patch adds an ARM
>> implementation for this hook that returns false if
>> 'arm_disable_literal_pool' is set to true and true otherwise.
>>
>> This version of the patch also reverts back to using the check for
>> 'SYMBOL_REF' in 'thumb2_legitimate_address_p' that was removed in the
>> last version, this code is required to place the label references in
>> rodata sections.
>>
>> Another thing this patch does is revert the changes made to the 32-bit
>> constant split in arm.md. The reason this was needed before was because
>> 'real_to_target' returns a long array and does not sign-extend values in
>> it, which would make sense on hosts with 64-bit longs. To fix this the
>> value is now casted to 'int' first. It would probably be a good idea to
>> change the 'real_to_target' function to return an array with
>> 'HOST_WIDE_INT' elements instead and either use all 64-bits or
>> sign-extend them. Something for the future?
>>
>> I added more test cases in this patch and reran regression tests for:
>> Cortex-M0, Cortex-M4 with and without -mslow-flash-data. Also did a
>> bootstrap+regressions on arm-none-linux-gnueabihf.
>>
>> Is this OK for trunk?
>>
>> Cheers,
>> Andre
>>
>> gcc/ChangeLog:
>>
>> 2016-11-29 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>
>> PR target/71607
>> * config/arm/arm.md (use_literal_pool): Removes.
>> (64-bit immediate split): No longer takes cost into consideration
>> if 'arm_disable_literal_pool' is enabled.
>> * config/arm/arm.c (arm_use_blocks_for_constant_p): New.
>> (TARGET_USE_BLOCKS_FOR_CONSTANT_P): Define.
>> (arm_max_const_double_inline_cost): Remove use of
>> arm_disable_literal_pool.
>> * config/arm/vfp.md (no_literal_pool_df_immediate): New.
>> (no_literal_pool_sf_immediate): New.
>>
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2016-11-29 Andre Vieira <andre.simoesdiasvieira@arm.com>
>> Thomas Preud'homme <thomas.preudhomme@arm.com>
>>
>> PR target/71607
>> * gcc.target/arm/thumb2-slow-flash-data.c: Renamed to ...
>> * gcc.target/arm/thumb2-slow-flash-data-1.c: ... this.
>> * gcc.target/arm/thumb2-slow-flash-data-2.c: New.
>> * gcc.target/arm/thumb2-slow-flash-data-3.c: New.
>> * gcc.target/arm/thumb2-slow-flash-data-4.c: New.
>> * gcc.target/arm/thumb2-slow-flash-data-5.c: New.
>>
> Hello,
>
> As I have mentioned in my commit message for the fix on embedded-6 (see
> https://gcc.gnu.org/ml/gcc-patches/2016-12/msg01304.html) I found an
> issue with this code when testing its backport on the embedded-6-branch.
>
> I misread the definition of the target hook
> TARGET_USE_BLOCKS_FOR_CONSTANT_P and it seems the way I implemented it
> before only changed code generation if the -mslow-flash-data option
> wasn't passed. I.e. I don't need to implement it to solve this issue.
> The other changes in this patch are sufficient...
>
> I reran regressions for arm-none-eabi-gcc with the following targets:
> Cortex-M0, Cortex-M3, Cortex-M7 and ARMv8-M Baseline. I also ran
> bootstraps and full regressions for arm-none-linux-gnueabihf both in ARM
> and THUMB mode. All green!
>
> Is this OK for trunk?
>
> Cheers,
> Andre
>
> gcc/ChangeLog:
>
> 2016-12-28 Andre Vieira <andre.simoesdiasvieira@arm.com>
>
> PR target/71607
> * config/arm/arm.md (use_literal_pool): Removes.
> (64-bit immediate split): No longer takes cost into consideration
> if 'arm_disable_literal_pool' is enabled.
> * config/arm/arm.c (arm_max_const_double_inline_cost): Remove use
> of arm_disable_literal_pool.
> * config/arm/vfp.md (no_literal_pool_df_immediate): New.
> (no_literal_pool_sf_immediate): New.
>
>
> gcc/testsuite/ChangeLog:
>
> 2016-12-28 Andre Vieira <andre.simoesdiasvieira@arm.com>
> Thomas Preud'homme <thomas.preudhomme@arm.com>
>
> PR target/71607
> * gcc.target/arm/thumb2-slow-flash-data.c: Renamed to ...
> * gcc.target/arm/thumb2-slow-flash-data-1.c: ... this.
> * gcc.target/arm/thumb2-slow-flash-data-2.c: New.
> * gcc.target/arm/thumb2-slow-flash-data-3.c: New.
> * gcc.target/arm/thumb2-slow-flash-data-4.c: New.
> * gcc.target/arm/thumb2-slow-flash-data-5.c: New.
>
Ping.
Cheers,
Andre