[Bug target/94538] [10 Regression] ICE: in extract_constrain_insn_cached, at recog.c:2223 (insn does not satisfy its constraints) with -mcpu=cortex-m23 -mslow-flash-data
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94538 --- Comment #16 from Wilco --- (In reply to Christophe Lyon from comment #15) > (In reply to Wilco from comment #14) > > (In reply to Christophe Lyon from comment #11) > > > (In reply to Wilco from comment #10) > > > > > Right, but the code is functional. > > > > It doesn't avoid the literal load from flash which is exactly what pure-code > > and slow-flash-data is all about. > > For f1 on M0, I can see: > .section.rodata.cst4,"aM",%progbits,4 > .align 2 > .LC0: > .word .LANCHOR0 > .section .text,"0x2006",%progbits > [...] > f1: > movsr3, #:upper8_15:#.LC0 > lslsr3, #8 > addsr3, #:upper0_7:#.LC0 > lslsr3, #8 > addsr3, #:lower8_15:#.LC0 > lslsr3, #8 > addsr3, #:lower0_7:#.LC0 > ldr r3, [r3]@ 6 [c=10 l=2] *thumb1_movsi_insn/8 > ldr r0, [r3]@ 7 [c=10 l=2] *thumb1_movsi_insn/8 > bx lr > [...] > .bss > .align 2 > .set.LANCHOR0,. + 0 > .type x, %object > .size x, 4 > x: > .space 4 > > So the 1st load is from .rodata.cst4 and the 2nd load is from bss, both of > which do not have the purecode bit set (unlike .text). Isn't that OK? No, it will create a lot of complaints and support queries due to the obvious regressions. It goes against the definition of pure-code and slow-flash-data which is to remove the literal loads. And given the sequence is already inefficient, we should do everything to remove the indirection which increases the codesize overhead by 75%... Another aspect that needs to be checked is that GCC correctly spills addresses and complex constants instead of rematerializing them. This is basic minimal quality that one expects for a feature like this.
[Bug target/94538] [10 Regression] ICE: in extract_constrain_insn_cached, at recog.c:2223 (insn does not satisfy its constraints) with -mcpu=cortex-m23 -mslow-flash-data
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94538 --- Comment #15 from Christophe Lyon --- (In reply to Wilco from comment #14) > (In reply to Christophe Lyon from comment #11) > > (In reply to Wilco from comment #10) > > > Right, but the code is functional. > > It doesn't avoid the literal load from flash which is exactly what pure-code > and slow-flash-data is all about. For f1 on M0, I can see: .section.rodata.cst4,"aM",%progbits,4 .align 2 .LC0: .word .LANCHOR0 .section .text,"0x2006",%progbits [...] f1: movsr3, #:upper8_15:#.LC0 lslsr3, #8 addsr3, #:upper0_7:#.LC0 lslsr3, #8 addsr3, #:lower8_15:#.LC0 lslsr3, #8 addsr3, #:lower0_7:#.LC0 ldr r3, [r3]@ 6 [c=10 l=2] *thumb1_movsi_insn/8 ldr r0, [r3]@ 7 [c=10 l=2] *thumb1_movsi_insn/8 bx lr [...] .bss .align 2 .set.LANCHOR0,. + 0 .type x, %object .size x, 4 x: .space 4 So the 1st load is from .rodata.cst4 and the 2nd load is from bss, both of which do not have the purecode bit set (unlike .text). Isn't that OK?
[Bug target/94538] [10 Regression] ICE: in extract_constrain_insn_cached, at recog.c:2223 (insn does not satisfy its constraints) with -mcpu=cortex-m23 -mslow-flash-data
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94538
--- Comment #14 from Wilco ---
(In reply to Christophe Lyon from comment #11)
> (In reply to Wilco from comment #10)
> Right, but the code is functional.
It doesn't avoid the literal load from flash which is exactly what pure-code
and slow-flash-data is all about.
That brings me to another question, why does -mslow-flash-data give an error
when used with Cortex-M0? If the goal was to make things more orthogonal then
surely we should not give an error and allow both options on all M-class cores.
> I believe this is expected: as I wrote in my commit message
> "CASE_VECTOR_PC_RELATIVE is now false with -mpure-code, to avoid generating
> invalid assembly code with differences from symbols from two different
> sections (the difference cannot be computed by the assembler)."
>
> Maybe there's a possibility to tune this to detect cases where we can do
> better?
The best option is to do the same as Cortex-M3: just switch off branch tables
altogether and fall back to compare+branch. That completely avoids loading data
from flash and is always smaller than emitting 32-bit tables.
> > Also relocations with a small offset should be handled within the
> > relocation. I'd expect this to never generate an extra addition, let alone
> > an extra literal pool entry:
> >
> > int arr[10];
> > int *f4 (void) { return [1]; }
> >
> > -O2 -mcpu=cortex-m3 -mpure-code generates the expected:
> >
> > movwr0, #:lower16:.LANCHOR0+4
> > movtr0, #:upper16:.LANCHOR0+4
> > bx lr
> >
> > -O2 -mcpu=cortex-m23 -mpure-code generates this:
> >
> > movwr0, #:lower16:.LANCHOR0
> > movtr0, #:upper16:.LANCHOR0
> > addsr0, r0, #4
> > bx lr
>
> For cortex-m23, I get the same code with and without -mpure-code.
GCC9 emits something different for Cortex-M0 and Cortex-M23 so this was changed
by the patch somehow even when -mpure-code is not enabled. So it is a
regression from what we used to generate.
Similarly I would not expect pure-code to change the decision of whether to
emit immediates as part of the relocation as long as they are within range. The
existing implementation for Cortex-M3 does this correctly.
[Bug target/94538] [10 Regression] ICE: in extract_constrain_insn_cached, at recog.c:2223 (insn does not satisfy its constraints) with -mcpu=cortex-m23 -mslow-flash-data
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94538 --- Comment #13 from Wilco --- (In reply to Christophe Lyon from comment #12) > I've posted a patch to fix the regression for your f3() examples: > https://gcc.gnu.org/pipermail/gcc-patches/2020-April/543993.html Yes that improves some of the examples, but I still see regressions on m0 for eg. -1 or 511. The splitter must have the right priority or it will continue to cause regressions. That means placing it around line 790 after the existing movsi splitters.
[Bug target/94538] [10 Regression] ICE: in extract_constrain_insn_cached, at recog.c:2223 (insn does not satisfy its constraints) with -mcpu=cortex-m23 -mslow-flash-data
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94538 --- Comment #12 from Christophe Lyon --- I've posted a patch to fix the regression for your f3() examples: https://gcc.gnu.org/pipermail/gcc-patches/2020-April/543993.html
[Bug target/94538] [10 Regression] ICE: in extract_constrain_insn_cached, at recog.c:2223 (insn does not satisfy its constraints) with -mcpu=cortex-m23 -mslow-flash-data
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94538
--- Comment #11 from Christophe Lyon ---
(In reply to Wilco from comment #10)
>
> For example:
>
> int x;
> int f1 (void) { return x; }
>
> with eg. -O2 -mcpu=cortex-m0 -mpure-code I get:
>
> movsr3, #:upper8_15:#.LC1
> lslsr3, #8
> addsr3, #:upper0_7:#.LC1
> lslsr3, #8
> addsr3, #:lower8_15:#.LC1
> lslsr3, #8
> addsr3, #:lower0_7:#.LC1
> @ sp needed
> ldr r3, [r3]
> ldr r0, [r3, #40]
> bx lr
>
> That's an extra indirection through a literal... There should only be
> one ldr to read x.
Right, but the code is functional. I mentioned that problem when I submitted
the patch. I thought it was good to provide functionality and improve the
generated code later.
I wrote: "I haven't found yet how to make code for cortex-m0 apply upper/lower
relocations to "p" instead of .LC2. The current code looks functional, but
could be improved."
>
> Big switch tables are produced for any Thumb-1 core, however I would expect
> Cortex-m0/m23 versions to look almost identical to the Cortex-m3 one, and
> use a sequence of comparisons instead of tables.
>
> 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;
> }
>
I believe this is expected: as I wrote in my commit message
"CASE_VECTOR_PC_RELATIVE is now false with -mpure-code, to avoid generating
invalid assembly code with differences from symbols from two different sections
(the difference cannot be computed by the assembler)."
Maybe there's a possibility to tune this to detect cases where we can do
better?
> Immediate generation for common cases seems to be screwed up:
>
> int f3 (void) { return 0x1100; }
>
> -O2 -mcpu=cortex-m0 -mpure-code:
>
> movsr0, #17
> lslsr0, r0, #8
> lslsr0, r0, #8
> lslsr0, r0, #8
> bx lr
This is not optimal, but functional, right?
> This also regressed Cortex-m23 which previously generated:
>
> movsr0, #136
> lslsr0, r0, #21
> bx lr
> Similar regressions happen with other immediates:
>
> int f3 (void) { return 0x12345678; }
>
> -O2 -mcpu=cortex-m23 -mpure-code:
>
> movsr0, #86
> lslsr0, r0, #8
> addsr0, r0, #120
> movtr0, 4660
> bx lr
>
> Previously it was:
>
> movwr0, #22136
> movtr0, 4660
> bx lr
OK, I'll check how to fix that.
> Also relocations with a small offset should be handled within the
> relocation. I'd expect this to never generate an extra addition, let alone
> an extra literal pool entry:
>
> int arr[10];
> int *f4 (void) { return [1]; }
>
> -O2 -mcpu=cortex-m3 -mpure-code generates the expected:
>
> movwr0, #:lower16:.LANCHOR0+4
> movtr0, #:upper16:.LANCHOR0+4
> bx lr
>
> -O2 -mcpu=cortex-m23 -mpure-code generates this:
>
> movwr0, #:lower16:.LANCHOR0
> movtr0, #:upper16:.LANCHOR0
> addsr0, r0, #4
> bx lr
For cortex-m23, I get the same code with and without -mpure-code.
>
> And cortex-m0 again inserts an extra literal load:
>
> movsr3, #:upper8_15:#.LC0
> lslsr3, #8
> addsr3, #:upper0_7:#.LC0
> lslsr3, #8
> addsr3, #:lower8_15:#.LC0
> lslsr3, #8
> addsr3, #:lower0_7:#.LC0
> ldr r0, [r3]
> addsr0, r0, #4
> bx lr
Yes, same problem as in f1()
So I think -mpure-code for v6m is not broken, but yes the generated code can be
improved. So this may not be relevant to this PR?
[Bug target/94538] [10 Regression] ICE: in extract_constrain_insn_cached, at recog.c:2223 (insn does not satisfy its constraints) with -mcpu=cortex-m23 -mslow-flash-data
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94538
--- Comment #10 from Wilco ---
(In reply to Christophe Lyon from comment #8)
> > Adding Christophe. I'm thinking the best approach right now is to revert
> > given -mpure-code doesn't work at all on Thumb-1 targets - it still emits
> > literal pools, switch tables etc. That's not pure code!
>
> Do you have testcases that show these failures?
>
> I did check some of the problematic testcases in the GCC testsuite when I
> committed that patch. Did I miss some of them?
>
> Can you point me to offending testcases and compiler options so that I can
> reproduce them?
For example:
int x;
int f1 (void) { return x; }
with eg. -O2 -mcpu=cortex-m0 -mpure-code I get:
movsr3, #:upper8_15:#.LC1
lslsr3, #8
addsr3, #:upper0_7:#.LC1
lslsr3, #8
addsr3, #:lower8_15:#.LC1
lslsr3, #8
addsr3, #:lower0_7:#.LC1
@ sp needed
ldr r3, [r3]
ldr r0, [r3, #40]
bx lr
That's an extra indirection through a literal... There should only be one ldr
to read x.
Big switch tables are produced for any Thumb-1 core, however I would expect
Cortex-m0/m23 versions to look almost identical to the Cortex-m3 one, and use a
sequence of comparisons instead of tables.
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;
}
Immediate generation for common cases seems to be screwed up:
int f3 (void) { return 0x1100; }
-O2 -mcpu=cortex-m0 -mpure-code:
movsr0, #17
lslsr0, r0, #8
lslsr0, r0, #8
lslsr0, r0, #8
bx lr
This also regressed Cortex-m23 which previously generated:
movsr0, #136
lslsr0, r0, #21
bx lr
Similar regressions happen with other immediates:
int f3 (void) { return 0x12345678; }
-O2 -mcpu=cortex-m23 -mpure-code:
movsr0, #86
lslsr0, r0, #8
addsr0, r0, #120
movtr0, 4660
bx lr
Previously it was:
movwr0, #22136
movtr0, 4660
bx lr
Also relocations with a small offset should be handled within the relocation.
I'd expect this to never generate an extra addition, let alone an extra literal
pool entry:
int arr[10];
int *f4 (void) { return [1]; }
-O2 -mcpu=cortex-m3 -mpure-code generates the expected:
movwr0, #:lower16:.LANCHOR0+4
movtr0, #:upper16:.LANCHOR0+4
bx lr
-O2 -mcpu=cortex-m23 -mpure-code generates this:
movwr0, #:lower16:.LANCHOR0
movtr0, #:upper16:.LANCHOR0
addsr0, r0, #4
bx lr
And cortex-m0 again inserts an extra literal load:
movsr3, #:upper8_15:#.LC0
lslsr3, #8
addsr3, #:upper0_7:#.LC0
lslsr3, #8
addsr3, #:lower8_15:#.LC0
lslsr3, #8
addsr3, #:lower0_7:#.LC0
ldr r0, [r3]
addsr0, r0, #4
bx lr
[Bug target/94538] [10 Regression] ICE: in extract_constrain_insn_cached, at recog.c:2223 (insn does not satisfy its constraints) with -mcpu=cortex-m23 -mslow-flash-data
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94538 --- Comment #9 from Zdenek Sojka --- Please don't take this PR serious; it might not be a valid use-case in real-world application.
[Bug target/94538] [10 Regression] ICE: in extract_constrain_insn_cached, at recog.c:2223 (insn does not satisfy its constraints) with -mcpu=cortex-m23 -mslow-flash-data
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94538 --- Comment #8 from Christophe Lyon --- > Adding Christophe. I'm thinking the best approach right now is to revert > given -mpure-code doesn't work at all on Thumb-1 targets - it still emits > literal pools, switch tables etc. That's not pure code! Do you have testcases that show these failures? I did check some of the problematic testcases in the GCC testsuite when I committed that patch. Did I miss some of them? Can you point me to offending testcases and compiler options so that I can reproduce them?
[Bug target/94538] [10 Regression] ICE: in extract_constrain_insn_cached, at recog.c:2223 (insn does not satisfy its constraints) with -mcpu=cortex-m23 -mslow-flash-data
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94538 --- Comment #7 from Wilco --- (In reply to Wilco from comment #4) > (In reply to Zdenek Sojka from comment #3) > > (In reply to Wilco from comment #2) > > > This was introduced by commit e24f6408d so only in GCC10. > > > > Thank you for checking this! > > > > I am quite sure this fails in gcc-9 as well: > ... > > Perhaps the offending commit, or part of it, was backported to gcc-9 as > > well? > > It's possible it was recently backported and our GCC9 builds don't yet have > it. But that whole patch is badly broken and introduces multiple issues... Adding Christophe. I'm thinking the best approach right now is to revert given -mpure-code doesn't work at all on Thumb-1 targets - it still emits literal pools, switch tables etc. That's not pure code!
[Bug target/94538] [10 Regression] ICE: in extract_constrain_insn_cached, at recog.c:2223 (insn does not satisfy its constraints) with -mcpu=cortex-m23 -mslow-flash-data
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94538 Iain Sandoe changed: What|Removed |Added CC||iains at gcc dot gnu.org --- Comment #6 from Iain Sandoe --- (In reply to CVS Commits from comment #5) > The master branch has been updated by Iain D Sandoe : sorry about the bad PR number, this commit is unrelated to the PR.
[Bug target/94538] [10 Regression] ICE: in extract_constrain_insn_cached, at recog.c:2223 (insn does not satisfy its constraints) with -mcpu=cortex-m23 -mslow-flash-data
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94538 --- Comment #5 from CVS Commits --- The master branch has been updated by Iain D Sandoe : https://gcc.gnu.org/g:0666767eb4cc864f00ba34d97b9d58f8dc650bba commit r10-7682-g0666767eb4cc864f00ba34d97b9d58f8dc650bba Author: Iain Sandoe Date: Fri Apr 10 20:55:10 2020 +0100 coroutines: Revise await expansions [PR94528] The expansions for await expressions were specific to particular cases, this revises it to be more generic. a: Revise co_await statement walkers. We want to process the co_awaits one statement at a time. We also want to be able to determine the insertion points for new bind scopes needed to cater for temporaries that are captured by reference and have lifetimes that need extension to the end of the full expression. Likewise, the handling of captured references in the evaluation of conditions might result in the need to make a frame copy. This reorganises the statement walking code to make it easier to extend for these purposes. b: Factor reference-captured temp code. We want to be able to use the code that writes a new bind expr with vars (and their initializers) from several places, so split that out of the maybe_promote_captured_temps() function into a new replace_statement_captures (). Update some comments. c: Generalize await statement expansion. This revises the expansion to avoid the need to expand conditionally on the tree type. It resolves PR 94528. gcc/cp/ChangeLog: 2020-04-10 Iain Sandoe PR c++/94538 * coroutines.cc (co_await_expander): Remove. (expand_one_await_expression): New. (process_one_statement): New. (await_statement_expander): New. (build_actor_fn): Revise to use per-statement expander. (struct susp_frame_data): Reorder and comment. (register_awaits): Factor code. (replace_statement_captures): New, factored from... (maybe_promote_captured_temps):.. here. (await_statement_walker): Revise to process per statement. (morph_fn_to_coro): Use revised susp_frame_data layout. gcc/testsuite/ChangeLog: 2020-04-10 Iain Sandoe PR c++/94538 * g++.dg/coroutines/pr94528.C: New test.
[Bug target/94538] [10 Regression] ICE: in extract_constrain_insn_cached, at recog.c:2223 (insn does not satisfy its constraints) with -mcpu=cortex-m23 -mslow-flash-data
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94538 --- Comment #4 from Wilco --- (In reply to Zdenek Sojka from comment #3) > (In reply to Wilco from comment #2) > > This was introduced by commit e24f6408d so only in GCC10. > > Thank you for checking this! > > I am quite sure this fails in gcc-9 as well: ... > Perhaps the offending commit, or part of it, was backported to gcc-9 as well? It's possible it was recently backported and our GCC9 builds don't yet have it. But that whole patch is badly broken and introduces multiple issues...
[Bug target/94538] [10 Regression] ICE: in extract_constrain_insn_cached, at recog.c:2223 (insn does not satisfy its constraints) with -mcpu=cortex-m23 -mslow-flash-data
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94538
--- Comment #3 from Zdenek Sojka ---
(In reply to Wilco from comment #2)
> This was introduced by commit e24f6408d so only in GCC10.
Thank you for checking this!
I am quite sure this fails in gcc-9 as well:
$ armv7a-hardfloat-linux-gnueabi-gcc -mcpu=cortex-m23 -mslow-flash-data
testcase.c
testcase.c: In function 'foo':
testcase.c:9:1: error: insn does not satisfy its constraints:
9 | }
| ^
(insn 91 90 17 (set (reg:SI 12 ip [125])
(plus:SI (reg:SI 12 ip [125])
(const_int 255 [0xff]))) "testcase.c":8:5 812 {*thumb1_addsi3}
(expr_list:REG_EQUAL (const_int 4095 [0xfff])
(nil)))
during RTL pass: shorten
testcase.c:9:1: internal compiler error: in extract_constrain_insn_cached, at
recog.c:2223
0x68bade _fatal_insn(char const*, rtx_def const*, char const*, int, char
const*)
/repo/gcc-9-branch/gcc/rtl-error.c:108
0x68bb6b _fatal_insn_not_found(rtx_def const*, char const*, int, char const*)
/repo/gcc-9-branch/gcc/rtl-error.c:118
0x67c6a2 extract_constrain_insn_cached(rtx_insn*)
/repo/gcc-9-branch/gcc/recog.c:2223
0x14555b3 insn_default_length(rtx_insn*)
/repo/gcc-9-branch/gcc/config/arm/arm.md:376
0xbc958e shorten_branches(rtx_insn*)
/repo/gcc-9-branch/gcc/final.c:1119
0xbc965f rest_of_handle_shorten_branches
/repo/gcc-9-branch/gcc/final.c:4754
0xbc965f execute
/repo/gcc-9-branch/gcc/final.c:4783
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See
[Bug target/94538] [10 Regression] ICE: in extract_constrain_insn_cached, at recog.c:2223 (insn does not satisfy its constraints) with -mcpu=cortex-m23 -mslow-flash-data
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94538 Wilco changed: What|Removed |Added Summary|[9/10 Regression] ICE: in |[10 Regression] ICE: in |extract_constrain_insn_cach |extract_constrain_insn_cach |ed, at recog.c:2223 (insn |ed, at recog.c:2223 (insn |does not satisfy its|does not satisfy its |constraints) with |constraints) with |-mcpu=cortex-m23|-mcpu=cortex-m23 |-mslow-flash-data |-mslow-flash-data --- Comment #2 from Wilco --- This was introduced by commit e24f6408d so only in GCC10.
