[PATCH] arm: Fix switch tables for thumb-1 with -mpure-code [PR96768]

[Richard Earnshaw]

On 28/08/2020 16:36, Christophe Lyon via Gcc-patches wrote:
> On Fri, 28 Aug 2020 at 16:27, Richard Earnshaw
> <Richard.Earnshaw@foss.arm.com> wrote:
>>
>> On 28/08/2020 14:24, Christophe Lyon via Gcc-patches wrote:
>>> On Fri, 28 Aug 2020 at 14:00, Richard Earnshaw
>>> <Richard.Earnshaw@foss.arm.com> wrote:
>>>>
>>>> On 27/08/2020 14:27, Christophe Lyon via Gcc-patches wrote:
>>>>> In comment 14 from PR94538, it was suggested to switch off jump tables
>>>>> on thumb-1 cores when using -mpure-code, like we already do for thumb-2.
>>>>>
>>>>> This is what this patch does, and also restores the previous value of
>>>>> CASE_VECTOR_PC_RELATIVE since it was not the right way of handling
>>>>> this.
>>>>>
>>>>> It also adds a new test, since the existing no-casesi.c did not catch
>>>>> this problem.
>>>>>
>>>>> Tested by running the whole testsuite with -mpure-code -mcpu=cortex-m0
>>>>> -mfloat-abi=soft, no regression and the new test passes (and fails
>>>>> without the fix).
>>>>>
>>>>> 2020-08-27  Christophe Lyon  <christophe.lyon@linaro.org>
>>>>>
>>>>>       gcc/
>>>>>       * config/arm/arm.h (CASE_VECTOR_PC_RELATIVE): Remove condition on
>>>>>       -mpure-code.
>>>>>       * config/arm/thumb1.md (tablejump): Disable when -mpure-code.
>>>>>
>>>>>       gcc/testsuite/
>>>>>       * gcc.target/arm/pure-code/pr96768.c: New test.
>>>>> ---
>>>>>  gcc/config/arm/arm.h                             |  5 ++---
>>>>>  gcc/config/arm/thumb1.md                         |  2 +-
>>>>>  gcc/testsuite/gcc.target/arm/pure-code/pr96768.c | 21 +++++++++++++++++++++
>>>>>  3 files changed, 24 insertions(+), 4 deletions(-)
>>>>>  create mode 100644 gcc/testsuite/gcc.target/arm/pure-code/pr96768.c
>>>>>
>>>>> diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
>>>>> index 3887c51..7d43721 100644
>>>>> --- a/gcc/config/arm/arm.h
>>>>> +++ b/gcc/config/arm/arm.h
>>>>> @@ -1975,10 +1975,9 @@ enum arm_auto_incmodes
>>>>>     for the index in the tablejump instruction.  */
>>>>>  #define CASE_VECTOR_MODE Pmode
>>>>>
>>>>> -#define CASE_VECTOR_PC_RELATIVE ((TARGET_THUMB2                              \
>>>>> +#define CASE_VECTOR_PC_RELATIVE (TARGET_THUMB2                               \
>>>>>                                 || (TARGET_THUMB1                     \
>>>>> -                                   && (optimize_size || flag_pic)))  \
>>>>> -                              && (!target_pure_code))
>>>>> +                                   && (optimize_size || flag_pic)))
>>>>>
>>>>>
>>>>>  #define CASE_VECTOR_SHORTEN_MODE(min, max, body)                     \
>>>>> diff --git a/gcc/config/arm/thumb1.md b/gcc/config/arm/thumb1.md
>>>>> index f0129db..602039e 100644
>>>>> --- a/gcc/config/arm/thumb1.md
>>>>> +++ b/gcc/config/arm/thumb1.md
>>>>> @@ -2012,7 +2012,7 @@ (define_insn "*epilogue_insns"
>>>>>  (define_expand "tablejump"
>>>>>    [(parallel [(set (pc) (match_operand:SI 0 "register_operand"))
>>>>>             (use (label_ref (match_operand 1 "" "")))])]
>>>>> -  "TARGET_THUMB1"
>>>>> +  "TARGET_THUMB1 && !target_pure_code"
>>>>>    "
>>>>>    if (flag_pic)
>>>>>      {
>>>>> diff --git a/gcc/testsuite/gcc.target/arm/pure-code/pr96768.c b/gcc/testsuite/gcc.target/arm/pure-code/pr96768.c
>>>>> new file mode 100644
>>>>> index 0000000..fd4cad5
>>>>> --- /dev/null
>>>>> +++ b/gcc/testsuite/gcc.target/arm/pure-code/pr96768.c
>>>>> @@ -0,0 +1,21 @@
>>>>> +/* { dg-do compile } */
>>>>> +/* { dg-options "-mpure-code" } */
>>>>> +
>>>>> +int f2 (int x, int y)
>>>>> +{
>>>>> +  switch (x)
>>>>> +    {
>>>>> +    case 0: return y + 0;
>>>>> +    case 1: return y + 1;
>>>>> +    case 2: return y + 2;
>>>>> +    case 3: return y + 3;
>>>>> +    case 4: return y + 4;
>>>>> +    case 5: return y + 5;
>>>>> +    }
>>>>> +  return y;
>>>>> +}
>>>>> +
>>>>> +/* We do not want any load from literal pool, but accept loads from r7
>>>>> +   (frame pointer, used at -O0).  */
>>>>> +/* { dg-final { scan-assembler-not "ldr\tr\\d+, \\\[r\[0-689\]+\\\]" } } */
>>>>> +/* { dg-final { scan-assembler "text,\"0x20000006\"" } } */
>>>>>
>>>>
>>>> Having switch tables is only a problem if they are embedded in the .text
>>>> segment.  But the case you show in the PR has them in the .rodata
>>>> segment, so is this really necessary?
>>>>
>>>
>>> Well, in the original PR94538, comment #14, Wilco said this was the best option.
>>>
>>
>> If the choice were not really a choice (ie pure code could not use
>> switch tables and still be pure) yes, it would be the case.  But that
>> isn't the case.
> 
> OK thanks, that was my initial impression.
> 
> So it seems this PR is actually not-a-bug/invalid?....
> 
> 
>>
>> GCC already knows generally when using jump sequences is going to be
>> better than switch tables and when tables are likely to be better.  It
>> can even produce a meld of the two when appropriate, it can even take
>> into account whether or not we are optimizing for size.  So we shouldn't
>> be just ride rough-shod over those choices.  Pure code doesn't really
>> change the balance.
> 
> 
> ... or should the PR be the other way around and request to improve how such
> cases are handled for thumb2? (ie do not disable arn.md:casesi completely
> with -mpure-code?)
> 
> 
> 
> 
>>
>> R.
>>
>>>> R.
>>

Might be better to reject this one and create a new enhancement PR.

R.