Re: [PATCH] arm: Fix switch tables for thumb-1 with -mpure-code [PR96768]
On 28/08/2020 16:36, Christophe Lyon via Gcc-patches wrote:
> On Fri, 28 Aug 2020 at 16:27, Richard Earnshaw
> wrote:
>>
>> On 28/08/2020 14:24, Christophe Lyon via Gcc-patches wrote:
>>> On Fri, 28 Aug 2020 at 14:00, Richard Earnshaw
>>> 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
>
> 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 000..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,\"0x2006\"" } } */
>
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
Re: [PATCH] arm: Fix switch tables for thumb-1 with -mpure-code [PR96768]
On Fri, 28 Aug 2020 at 16:27, Richard Earnshaw
wrote:
>
> On 28/08/2020 14:24, Christophe Lyon via Gcc-patches wrote:
> > On Fri, 28 Aug 2020 at 14:00, Richard Earnshaw
> > 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
> >>>
> >>> 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 000..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,\"0x2006\"" } } */
> >>>
> >>
> >> 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
Re: [PATCH] arm: Fix switch tables for thumb-1 with -mpure-code [PR96768]
On 28/08/2020 15:27, Richard Earnshaw wrote:
> On 28/08/2020 14:24, Christophe Lyon via Gcc-patches wrote:
>> On Fri, 28 Aug 2020 at 14:00, Richard Earnshaw
>> 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
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 000..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,\"0x2006\"" } } */
>>>
>>> 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.
>
> 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.
>
> R.
>
>>> R.
>
Also note that it would be possible to compress the data tables in a
similar way to the aarch64 port; there's no need for the entries to be
32 bits most of the time. It just needs someone to do the work.
R.
Re: [PATCH] arm: Fix switch tables for thumb-1 with -mpure-code [PR96768]
On 28/08/2020 14:24, Christophe Lyon via Gcc-patches wrote:
> On Fri, 28 Aug 2020 at 14:00, Richard Earnshaw
> 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
>>>
>>> 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 000..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,\"0x2006\"" } } */
>>>
>>
>> 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.
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.
R.
>> R.
Re: [PATCH] arm: Fix switch tables for thumb-1 with -mpure-code [PR96768]
On Fri, 28 Aug 2020 at 14:00, Richard Earnshaw
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
> >
> > 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 000..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,\"0x2006\"" } } */
> >
>
> 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.
> R.
Re: [PATCH] arm: Fix switch tables for thumb-1 with -mpure-code [PR96768]
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
>
> 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 000..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,\"0x2006\"" } } */
>
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?
R.
