Hi,
On 5 May 2017 at 15:19, Richard Earnshaw (lists)
<richard.earns...@arm.com> wrote:
> On 04/05/17 11:40, Prakhar Bahuguna wrote:
>> On 03/05/2017 11:30:13, Richard Earnshaw (lists) wrote:
>>> On 20/04/17 10:54, Prakhar Bahuguna wrote:
>>>> [ARM] PR71607: Fix ICE when loading constant
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> 2017-04-18 Andre Vieira <andre.simoesdiasvie...@arm.com>
>>>> Prakhar Bahuguna <prakhar.bahug...@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_tls_referenced_p): Add diagnostic if TLS is
>>>> used when arm_disable_literal_pool is enabled.
>>>> (arm_max_const_double_inline_cost): Remove use of
>>>> arm_disable_literal_pool.
>>>> (arm_reorg): Add return if arm_disable_literal_pool is enabled.
>>>> * config/arm/vfp.md (no_literal_pool_df_immediate): New.
>>>> (no_literal_pool_sf_immediate): New.
>>>>
>>>> testsuite/ChangeLog:
>>>>
>>>> 2017-04-18 Andre Vieira <andre.simoesdiasvie...@arm.com>
>>>> Thomas Preud'homme <thomas.preudho...@arm.com>
>>>> Prakhar Bahuguna <prakhar.bahug...@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.
>>>> * gcc.target/arm/tls-disable-literal-pool.c: New.
I've noticed that the last new test (tls-disable-literal-pool.c)
fails on arm-eabi --with-mode=thumb --with-cpu=cortex-m3:
no error message is generated.
Thanks,
Christophe
>>>>
>>>> Okay for stage1?
>>>>
>>>
>>> This patch lacks a description of what's going on and why the change is
>>> necessary (it should stand alone from the PR data). It's clearly a
>>> non-trivial change, so why have you adopted this approach?
>>>
>>> R.
>>>
>>
>> Hi,
>>
>> This patch is based off an earlier patch that was applied to the
>> embedded-6-branch, and I had neglected to include the full description, which
>> is presented below:
>>
>> 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.
>>
>> Additionally, the use of thread-local storage is disabled if literal pools
>> are
>> disabled, as there are no relocations for TLS variables and incorrect code is
>> generated as a result. The patch now emits a diagnostic in TLS-enabled
>> toolchains if a TLS symbol is found when -mpure-code or -mslow-flash-data are
>> enabled.
>>
>
> Thanks, that helps a lot.
>
> + {
> + /* ARM currently does not provide relocations to encode TLS
> variables
>
> ARM ELF does not define relocations ...
>
> + /* Make sure we do not attempt to create a literal pool even though
> it should
> + no longer be necessary to create any. */
> + if (arm_disable_literal_pool)
> + return ;
> +
>
> It would be safer to run through the code and then assert that fixups
> aren't needed; though that would cost a little computation time. I
> think you could put such an assert at the start of push_minipool_fix.
>
> OK with those changes.
>
> R.
