[PATCH, GCC/ARM, Stage 1] PR71607: Fix ICE when loading constant

[Christophe Lyon]

Hi,


On 5 May 2017 at 15:19, Richard Earnshaw (lists)
<Richard.Earnshaw@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.simoesdiasvieira@arm.com>
>>>>         Prakhar Bahuguna  <prakhar.bahuguna@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.simoesdiasvieira@arm.com>
>>>>         Thomas Preud'homme  <thomas.preudhomme@arm.com>
>>>>         Prakhar Bahuguna  <prakhar.bahuguna@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.