Re: [PATCH] arm: Fix -mpure-code support/-mslow-flash-data for armv8-m.base [PR94538]

2020-08-27 Thread Christophe Lyon via Gcc-patches
On Thu, 27 Aug 2020 at 11:02, Ramana Radhakrishnan
 wrote:
>
> On Mon, Aug 24, 2020 at 4:35 PM Christophe Lyon
>  wrote:
> >
> > On Mon, 24 Aug 2020 at 11:09, Christophe Lyon
> >  wrote:
> > >
> > > On Sat, 22 Aug 2020 at 00:44, Ramana Radhakrishnan
> > >  wrote:
> > > >
> > > > On Wed, Aug 19, 2020 at 10:32 AM Christophe Lyon via Gcc-patches
> > > >  wrote:
> > > > >
> > > > > armv8-m.base (cortex-m23) has the movt instruction, so we need to
> > > > > disable the define_split to generate a constant in this case,
> > > > > otherwise we get incorrect insn constraints as described in PR94538.
> > > > >
> > > > > We also need to fix the pure-code alternative for thumb1_movsi_insn
> > > > > because the assembler complains with instructions like
> > > > > movs r0, #:upper8_15:1234
> > > > > (Internal error in md_apply_fix)
> > > > > We now generate movs r0, 4 instead.
> > > > >
> > > > > 2020-08-19  Christophe Lyon  
> > > > > gcc/ChangeLog:
> > > > >
> > > > > * config/arm/thumb1.md: Disable set-constant splitter when
> > > > > TARGET_HAVE_MOVT.
> > > > > (thumb1_movsi_insn): Fix -mpure-code
> > > > > alternative.
> > > > >
> > > > > gcc/testsuite/ChangeLog:
> > > > >
> > > > > * gcc.target/arm/pure-code/pr94538-1.c: New test.
> > > > > * gcc.target/arm/pure-code/pr94538-2.c: New test.
> > > >
> > >
> > > Hi Ramana,
> > >
> > > >  I take it that this fixes the ICE rather than addressing the code
> > > > generation / performance bits that Wilco was referring to ? Really the
> > > > other code quality / performance issues listed in the PR should really
> > > > be broken down into separate PRs while we take this as a fix for
> > > > fixing the ICE,
> > > >
> > > > Under that assumption OK.
> > > >
> > >
> > > Yes, that's correct, this patch just fixes the ICE, as it is cleaner
> > > to handle perf issues in a different patch.
> > >
> >
> > The patch applies cleanly to gcc-9 and gcc-10: OK to backport there?
>
> Yes, all good.
>

Thanks, I just pushed the two backports.

Christophe

> Thanks,
> Ramana
>
> >
> > (I tested that the offending testcase passes on the branches with the
> > backport, and fails without).
> >
> > Thanks,
> >
> > Christophe
> >
> > > I'll open a new PR to track the perf issue.
> > >
> > > Thanks
> > >
> > >
> > > > Ramana
> > > >
> > > > > ---
> > > > >  gcc/config/arm/thumb1.md   | 66 
> > > > > ++
> > > > >  gcc/testsuite/gcc.target/arm/pure-code/pr94538-1.c | 13 +
> > > > >  gcc/testsuite/gcc.target/arm/pure-code/pr94538-2.c | 12 
> > > > >  3 files changed, 79 insertions(+), 12 deletions(-)
> > > > >  create mode 100644 gcc/testsuite/gcc.target/arm/pure-code/pr94538-1.c
> > > > >  create mode 100644 gcc/testsuite/gcc.target/arm/pure-code/pr94538-2.c
> > > > >
> > > > > diff --git a/gcc/config/arm/thumb1.md b/gcc/config/arm/thumb1.md
> > > > > index 0ff8190..f0129db 100644
> > > > > --- a/gcc/config/arm/thumb1.md
> > > > > +++ b/gcc/config/arm/thumb1.md
> > > > > @@ -70,6 +70,7 @@ (define_split
> > > > >"TARGET_THUMB1
> > > > > && arm_disable_literal_pool
> > > > > && GET_CODE (operands[1]) == CONST_INT
> > > > > +   && !TARGET_HAVE_MOVT
> > > > > && !satisfies_constraint_I (operands[1])"
> > > > >[(clobber (const_int 0))]
> > > > >"
> > > > > @@ -696,18 +697,59 @@ (define_insn "*thumb1_movsi_insn"
> > > > >"TARGET_THUMB1
> > > > > && (   register_operand (operands[0], SImode)
> > > > > || register_operand (operands[1], SImode))"
> > > > > -  "@
> > > > > -   movs%0, %1
> > > > > -   movs%0, %1
> > > > > -   movw%0, %1
> > > > > -   #
> > > > > -   #
> > > > > -   ldmia\\t%1, {%0}
> > > > > -   stmia\\t%0, {%1}
> > > > > -   movs\\t%0, #:upper8_15:%1; lsls\\t%0, #8; adds\\t%0, 
> > > > > #:upper0_7:%1; lsls\\t%0, #8; adds\\t%0, #:lower8_15:%1; lsls\\t%0, 
> > > > > #8; adds\\t%0, #:lower0_7:%1
> > > > > -   ldr\\t%0, %1
> > > > > -   str\\t%1, %0
> > > > > -   mov\\t%0, %1"
> > > > > +{
> > > > > +  switch (which_alternative)
> > > > > +{
> > > > > +  default:
> > > > > +  case 0: return "movs\t%0, %1";
> > > > > +  case 1: return "movs\t%0, %1";
> > > > > +  case 2: return "movw\t%0, %1";
> > > > > +  case 3: return "#";
> > > > > +  case 4: return "#";
> > > > > +  case 5: return "ldmia\t%1, {%0}";
> > > > > +  case 6: return "stmia\t%0, {%1}";
> > > > > +  case 7:
> > > > > +  /* pure-code alternative: build the constant byte by byte,
> > > > > +instead of loading it from a constant pool.  */
> > > > > +   {
> > > > > + int i;
> > > > > + HOST_WIDE_INT op1 = INTVAL (operands[1]);
> > > > > + bool mov_done_p = false;
> > > > > + rtx ops[2];
> > > > > + ops[0] = operands[0];
> > > > > +
> > > > > + /* Emit upper 3 bytes if needed.  */
> > > > > + for (i = 0; i < 3; i++)
> > > > > +   {
> > > > > +   

Re: [PATCH] arm: Fix -mpure-code support/-mslow-flash-data for armv8-m.base [PR94538]

2020-08-27 Thread Ramana Radhakrishnan via Gcc-patches
On Mon, Aug 24, 2020 at 4:35 PM Christophe Lyon
 wrote:
>
> On Mon, 24 Aug 2020 at 11:09, Christophe Lyon
>  wrote:
> >
> > On Sat, 22 Aug 2020 at 00:44, Ramana Radhakrishnan
> >  wrote:
> > >
> > > On Wed, Aug 19, 2020 at 10:32 AM Christophe Lyon via Gcc-patches
> > >  wrote:
> > > >
> > > > armv8-m.base (cortex-m23) has the movt instruction, so we need to
> > > > disable the define_split to generate a constant in this case,
> > > > otherwise we get incorrect insn constraints as described in PR94538.
> > > >
> > > > We also need to fix the pure-code alternative for thumb1_movsi_insn
> > > > because the assembler complains with instructions like
> > > > movs r0, #:upper8_15:1234
> > > > (Internal error in md_apply_fix)
> > > > We now generate movs r0, 4 instead.
> > > >
> > > > 2020-08-19  Christophe Lyon  
> > > > gcc/ChangeLog:
> > > >
> > > > * config/arm/thumb1.md: Disable set-constant splitter when
> > > > TARGET_HAVE_MOVT.
> > > > (thumb1_movsi_insn): Fix -mpure-code
> > > > alternative.
> > > >
> > > > gcc/testsuite/ChangeLog:
> > > >
> > > > * gcc.target/arm/pure-code/pr94538-1.c: New test.
> > > > * gcc.target/arm/pure-code/pr94538-2.c: New test.
> > >
> >
> > Hi Ramana,
> >
> > >  I take it that this fixes the ICE rather than addressing the code
> > > generation / performance bits that Wilco was referring to ? Really the
> > > other code quality / performance issues listed in the PR should really
> > > be broken down into separate PRs while we take this as a fix for
> > > fixing the ICE,
> > >
> > > Under that assumption OK.
> > >
> >
> > Yes, that's correct, this patch just fixes the ICE, as it is cleaner
> > to handle perf issues in a different patch.
> >
>
> The patch applies cleanly to gcc-9 and gcc-10: OK to backport there?

Yes, all good.

Thanks,
Ramana

>
> (I tested that the offending testcase passes on the branches with the
> backport, and fails without).
>
> Thanks,
>
> Christophe
>
> > I'll open a new PR to track the perf issue.
> >
> > Thanks
> >
> >
> > > Ramana
> > >
> > > > ---
> > > >  gcc/config/arm/thumb1.md   | 66 
> > > > ++
> > > >  gcc/testsuite/gcc.target/arm/pure-code/pr94538-1.c | 13 +
> > > >  gcc/testsuite/gcc.target/arm/pure-code/pr94538-2.c | 12 
> > > >  3 files changed, 79 insertions(+), 12 deletions(-)
> > > >  create mode 100644 gcc/testsuite/gcc.target/arm/pure-code/pr94538-1.c
> > > >  create mode 100644 gcc/testsuite/gcc.target/arm/pure-code/pr94538-2.c
> > > >
> > > > diff --git a/gcc/config/arm/thumb1.md b/gcc/config/arm/thumb1.md
> > > > index 0ff8190..f0129db 100644
> > > > --- a/gcc/config/arm/thumb1.md
> > > > +++ b/gcc/config/arm/thumb1.md
> > > > @@ -70,6 +70,7 @@ (define_split
> > > >"TARGET_THUMB1
> > > > && arm_disable_literal_pool
> > > > && GET_CODE (operands[1]) == CONST_INT
> > > > +   && !TARGET_HAVE_MOVT
> > > > && !satisfies_constraint_I (operands[1])"
> > > >[(clobber (const_int 0))]
> > > >"
> > > > @@ -696,18 +697,59 @@ (define_insn "*thumb1_movsi_insn"
> > > >"TARGET_THUMB1
> > > > && (   register_operand (operands[0], SImode)
> > > > || register_operand (operands[1], SImode))"
> > > > -  "@
> > > > -   movs%0, %1
> > > > -   movs%0, %1
> > > > -   movw%0, %1
> > > > -   #
> > > > -   #
> > > > -   ldmia\\t%1, {%0}
> > > > -   stmia\\t%0, {%1}
> > > > -   movs\\t%0, #:upper8_15:%1; lsls\\t%0, #8; adds\\t%0, #:upper0_7:%1; 
> > > > lsls\\t%0, #8; adds\\t%0, #:lower8_15:%1; lsls\\t%0, #8; adds\\t%0, 
> > > > #:lower0_7:%1
> > > > -   ldr\\t%0, %1
> > > > -   str\\t%1, %0
> > > > -   mov\\t%0, %1"
> > > > +{
> > > > +  switch (which_alternative)
> > > > +{
> > > > +  default:
> > > > +  case 0: return "movs\t%0, %1";
> > > > +  case 1: return "movs\t%0, %1";
> > > > +  case 2: return "movw\t%0, %1";
> > > > +  case 3: return "#";
> > > > +  case 4: return "#";
> > > > +  case 5: return "ldmia\t%1, {%0}";
> > > > +  case 6: return "stmia\t%0, {%1}";
> > > > +  case 7:
> > > > +  /* pure-code alternative: build the constant byte by byte,
> > > > +instead of loading it from a constant pool.  */
> > > > +   {
> > > > + int i;
> > > > + HOST_WIDE_INT op1 = INTVAL (operands[1]);
> > > > + bool mov_done_p = false;
> > > > + rtx ops[2];
> > > > + ops[0] = operands[0];
> > > > +
> > > > + /* Emit upper 3 bytes if needed.  */
> > > > + for (i = 0; i < 3; i++)
> > > > +   {
> > > > +  int byte = (op1 >> (8 * (3 - i))) & 0xff;
> > > > +
> > > > + if (byte)
> > > > +   {
> > > > + ops[1] = GEN_INT (byte);
> > > > + if (mov_done_p)
> > > > +   output_asm_insn ("adds\t%0, %1", ops);
> > > > + else
> > > > +   output_asm_insn ("movs\t%0, %1", ops);
> > > > +   

Re: [PATCH] arm: Fix -mpure-code support/-mslow-flash-data for armv8-m.base [PR94538]

2020-08-24 Thread Christophe Lyon via Gcc-patches
On Mon, 24 Aug 2020 at 11:09, Christophe Lyon
 wrote:
>
> On Sat, 22 Aug 2020 at 00:44, Ramana Radhakrishnan
>  wrote:
> >
> > On Wed, Aug 19, 2020 at 10:32 AM Christophe Lyon via Gcc-patches
> >  wrote:
> > >
> > > armv8-m.base (cortex-m23) has the movt instruction, so we need to
> > > disable the define_split to generate a constant in this case,
> > > otherwise we get incorrect insn constraints as described in PR94538.
> > >
> > > We also need to fix the pure-code alternative for thumb1_movsi_insn
> > > because the assembler complains with instructions like
> > > movs r0, #:upper8_15:1234
> > > (Internal error in md_apply_fix)
> > > We now generate movs r0, 4 instead.
> > >
> > > 2020-08-19  Christophe Lyon  
> > > gcc/ChangeLog:
> > >
> > > * config/arm/thumb1.md: Disable set-constant splitter when
> > > TARGET_HAVE_MOVT.
> > > (thumb1_movsi_insn): Fix -mpure-code
> > > alternative.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > > * gcc.target/arm/pure-code/pr94538-1.c: New test.
> > > * gcc.target/arm/pure-code/pr94538-2.c: New test.
> >
>
> Hi Ramana,
>
> >  I take it that this fixes the ICE rather than addressing the code
> > generation / performance bits that Wilco was referring to ? Really the
> > other code quality / performance issues listed in the PR should really
> > be broken down into separate PRs while we take this as a fix for
> > fixing the ICE,
> >
> > Under that assumption OK.
> >
>
> Yes, that's correct, this patch just fixes the ICE, as it is cleaner
> to handle perf issues in a different patch.
>

The patch applies cleanly to gcc-9 and gcc-10: OK to backport there?

(I tested that the offending testcase passes on the branches with the
backport, and fails without).

Thanks,

Christophe

> I'll open a new PR to track the perf issue.
>
> Thanks
>
>
> > Ramana
> >
> > > ---
> > >  gcc/config/arm/thumb1.md   | 66 
> > > ++
> > >  gcc/testsuite/gcc.target/arm/pure-code/pr94538-1.c | 13 +
> > >  gcc/testsuite/gcc.target/arm/pure-code/pr94538-2.c | 12 
> > >  3 files changed, 79 insertions(+), 12 deletions(-)
> > >  create mode 100644 gcc/testsuite/gcc.target/arm/pure-code/pr94538-1.c
> > >  create mode 100644 gcc/testsuite/gcc.target/arm/pure-code/pr94538-2.c
> > >
> > > diff --git a/gcc/config/arm/thumb1.md b/gcc/config/arm/thumb1.md
> > > index 0ff8190..f0129db 100644
> > > --- a/gcc/config/arm/thumb1.md
> > > +++ b/gcc/config/arm/thumb1.md
> > > @@ -70,6 +70,7 @@ (define_split
> > >"TARGET_THUMB1
> > > && arm_disable_literal_pool
> > > && GET_CODE (operands[1]) == CONST_INT
> > > +   && !TARGET_HAVE_MOVT
> > > && !satisfies_constraint_I (operands[1])"
> > >[(clobber (const_int 0))]
> > >"
> > > @@ -696,18 +697,59 @@ (define_insn "*thumb1_movsi_insn"
> > >"TARGET_THUMB1
> > > && (   register_operand (operands[0], SImode)
> > > || register_operand (operands[1], SImode))"
> > > -  "@
> > > -   movs%0, %1
> > > -   movs%0, %1
> > > -   movw%0, %1
> > > -   #
> > > -   #
> > > -   ldmia\\t%1, {%0}
> > > -   stmia\\t%0, {%1}
> > > -   movs\\t%0, #:upper8_15:%1; lsls\\t%0, #8; adds\\t%0, #:upper0_7:%1; 
> > > lsls\\t%0, #8; adds\\t%0, #:lower8_15:%1; lsls\\t%0, #8; adds\\t%0, 
> > > #:lower0_7:%1
> > > -   ldr\\t%0, %1
> > > -   str\\t%1, %0
> > > -   mov\\t%0, %1"
> > > +{
> > > +  switch (which_alternative)
> > > +{
> > > +  default:
> > > +  case 0: return "movs\t%0, %1";
> > > +  case 1: return "movs\t%0, %1";
> > > +  case 2: return "movw\t%0, %1";
> > > +  case 3: return "#";
> > > +  case 4: return "#";
> > > +  case 5: return "ldmia\t%1, {%0}";
> > > +  case 6: return "stmia\t%0, {%1}";
> > > +  case 7:
> > > +  /* pure-code alternative: build the constant byte by byte,
> > > +instead of loading it from a constant pool.  */
> > > +   {
> > > + int i;
> > > + HOST_WIDE_INT op1 = INTVAL (operands[1]);
> > > + bool mov_done_p = false;
> > > + rtx ops[2];
> > > + ops[0] = operands[0];
> > > +
> > > + /* Emit upper 3 bytes if needed.  */
> > > + for (i = 0; i < 3; i++)
> > > +   {
> > > +  int byte = (op1 >> (8 * (3 - i))) & 0xff;
> > > +
> > > + if (byte)
> > > +   {
> > > + ops[1] = GEN_INT (byte);
> > > + if (mov_done_p)
> > > +   output_asm_insn ("adds\t%0, %1", ops);
> > > + else
> > > +   output_asm_insn ("movs\t%0, %1", ops);
> > > + mov_done_p = true;
> > > +   }
> > > +
> > > + if (mov_done_p)
> > > +   output_asm_insn ("lsls\t%0, #8", ops);
> > > +   }
> > > +
> > > + /* Emit lower byte if needed.  */
> > > + ops[1] = GEN_INT (op1 & 0xff);
> > > + if (!mov_done_p)
> > > +   output_asm_insn 

Re: [PATCH] arm: Fix -mpure-code support/-mslow-flash-data for armv8-m.base [PR94538]

2020-08-24 Thread Christophe Lyon via Gcc-patches
On Sat, 22 Aug 2020 at 00:44, Ramana Radhakrishnan
 wrote:
>
> On Wed, Aug 19, 2020 at 10:32 AM Christophe Lyon via Gcc-patches
>  wrote:
> >
> > armv8-m.base (cortex-m23) has the movt instruction, so we need to
> > disable the define_split to generate a constant in this case,
> > otherwise we get incorrect insn constraints as described in PR94538.
> >
> > We also need to fix the pure-code alternative for thumb1_movsi_insn
> > because the assembler complains with instructions like
> > movs r0, #:upper8_15:1234
> > (Internal error in md_apply_fix)
> > We now generate movs r0, 4 instead.
> >
> > 2020-08-19  Christophe Lyon  
> > gcc/ChangeLog:
> >
> > * config/arm/thumb1.md: Disable set-constant splitter when
> > TARGET_HAVE_MOVT.
> > (thumb1_movsi_insn): Fix -mpure-code
> > alternative.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.target/arm/pure-code/pr94538-1.c: New test.
> > * gcc.target/arm/pure-code/pr94538-2.c: New test.
>

Hi Ramana,

>  I take it that this fixes the ICE rather than addressing the code
> generation / performance bits that Wilco was referring to ? Really the
> other code quality / performance issues listed in the PR should really
> be broken down into separate PRs while we take this as a fix for
> fixing the ICE,
>
> Under that assumption OK.
>

Yes, that's correct, this patch just fixes the ICE, as it is cleaner
to handle perf issues in a different patch.

I'll open a new PR to track the perf issue.

Thanks


> Ramana
>
> > ---
> >  gcc/config/arm/thumb1.md   | 66 
> > ++
> >  gcc/testsuite/gcc.target/arm/pure-code/pr94538-1.c | 13 +
> >  gcc/testsuite/gcc.target/arm/pure-code/pr94538-2.c | 12 
> >  3 files changed, 79 insertions(+), 12 deletions(-)
> >  create mode 100644 gcc/testsuite/gcc.target/arm/pure-code/pr94538-1.c
> >  create mode 100644 gcc/testsuite/gcc.target/arm/pure-code/pr94538-2.c
> >
> > diff --git a/gcc/config/arm/thumb1.md b/gcc/config/arm/thumb1.md
> > index 0ff8190..f0129db 100644
> > --- a/gcc/config/arm/thumb1.md
> > +++ b/gcc/config/arm/thumb1.md
> > @@ -70,6 +70,7 @@ (define_split
> >"TARGET_THUMB1
> > && arm_disable_literal_pool
> > && GET_CODE (operands[1]) == CONST_INT
> > +   && !TARGET_HAVE_MOVT
> > && !satisfies_constraint_I (operands[1])"
> >[(clobber (const_int 0))]
> >"
> > @@ -696,18 +697,59 @@ (define_insn "*thumb1_movsi_insn"
> >"TARGET_THUMB1
> > && (   register_operand (operands[0], SImode)
> > || register_operand (operands[1], SImode))"
> > -  "@
> > -   movs%0, %1
> > -   movs%0, %1
> > -   movw%0, %1
> > -   #
> > -   #
> > -   ldmia\\t%1, {%0}
> > -   stmia\\t%0, {%1}
> > -   movs\\t%0, #:upper8_15:%1; lsls\\t%0, #8; adds\\t%0, #:upper0_7:%1; 
> > lsls\\t%0, #8; adds\\t%0, #:lower8_15:%1; lsls\\t%0, #8; adds\\t%0, 
> > #:lower0_7:%1
> > -   ldr\\t%0, %1
> > -   str\\t%1, %0
> > -   mov\\t%0, %1"
> > +{
> > +  switch (which_alternative)
> > +{
> > +  default:
> > +  case 0: return "movs\t%0, %1";
> > +  case 1: return "movs\t%0, %1";
> > +  case 2: return "movw\t%0, %1";
> > +  case 3: return "#";
> > +  case 4: return "#";
> > +  case 5: return "ldmia\t%1, {%0}";
> > +  case 6: return "stmia\t%0, {%1}";
> > +  case 7:
> > +  /* pure-code alternative: build the constant byte by byte,
> > +instead of loading it from a constant pool.  */
> > +   {
> > + int i;
> > + HOST_WIDE_INT op1 = INTVAL (operands[1]);
> > + bool mov_done_p = false;
> > + rtx ops[2];
> > + ops[0] = operands[0];
> > +
> > + /* Emit upper 3 bytes if needed.  */
> > + for (i = 0; i < 3; i++)
> > +   {
> > +  int byte = (op1 >> (8 * (3 - i))) & 0xff;
> > +
> > + if (byte)
> > +   {
> > + ops[1] = GEN_INT (byte);
> > + if (mov_done_p)
> > +   output_asm_insn ("adds\t%0, %1", ops);
> > + else
> > +   output_asm_insn ("movs\t%0, %1", ops);
> > + mov_done_p = true;
> > +   }
> > +
> > + if (mov_done_p)
> > +   output_asm_insn ("lsls\t%0, #8", ops);
> > +   }
> > +
> > + /* Emit lower byte if needed.  */
> > + ops[1] = GEN_INT (op1 & 0xff);
> > + if (!mov_done_p)
> > +   output_asm_insn ("movs\t%0, %1", ops);
> > + else if (op1 & 0xff)
> > +   output_asm_insn ("adds\t%0, %1", ops);
> > + return "";
> > +   }
> > +  case 8: return "ldr\t%0, %1";
> > +  case 9: return "str\t%1, %0";
> > +  case 10: return "mov\t%0, %1";
> > +}
> > +}
> >[(set_attr "length" "2,2,4,4,4,2,2,14,2,2,2")
> > (set_attr "type" 
> > "mov_reg,mov_imm,mov_imm,multiple,multiple,load_4,store_4,alu_sreg,load_4,store_4,mov_reg")
> > (set_attr "pool_range" "*,*,*,*,*,*,*, 

Re: [PATCH] arm: Fix -mpure-code support/-mslow-flash-data for armv8-m.base [PR94538]

2020-08-21 Thread Ramana Radhakrishnan via Gcc-patches
On Wed, Aug 19, 2020 at 10:32 AM Christophe Lyon via Gcc-patches
 wrote:
>
> armv8-m.base (cortex-m23) has the movt instruction, so we need to
> disable the define_split to generate a constant in this case,
> otherwise we get incorrect insn constraints as described in PR94538.
>
> We also need to fix the pure-code alternative for thumb1_movsi_insn
> because the assembler complains with instructions like
> movs r0, #:upper8_15:1234
> (Internal error in md_apply_fix)
> We now generate movs r0, 4 instead.
>
> 2020-08-19  Christophe Lyon  
> gcc/ChangeLog:
>
> * config/arm/thumb1.md: Disable set-constant splitter when
> TARGET_HAVE_MOVT.
> (thumb1_movsi_insn): Fix -mpure-code
> alternative.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/arm/pure-code/pr94538-1.c: New test.
> * gcc.target/arm/pure-code/pr94538-2.c: New test.

 I take it that this fixes the ICE rather than addressing the code
generation / performance bits that Wilco was referring to ? Really the
other code quality / performance issues listed in the PR should really
be broken down into separate PRs while we take this as a fix for
fixing the ICE,

Under that assumption OK.

Ramana

> ---
>  gcc/config/arm/thumb1.md   | 66 
> ++
>  gcc/testsuite/gcc.target/arm/pure-code/pr94538-1.c | 13 +
>  gcc/testsuite/gcc.target/arm/pure-code/pr94538-2.c | 12 
>  3 files changed, 79 insertions(+), 12 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/arm/pure-code/pr94538-1.c
>  create mode 100644 gcc/testsuite/gcc.target/arm/pure-code/pr94538-2.c
>
> diff --git a/gcc/config/arm/thumb1.md b/gcc/config/arm/thumb1.md
> index 0ff8190..f0129db 100644
> --- a/gcc/config/arm/thumb1.md
> +++ b/gcc/config/arm/thumb1.md
> @@ -70,6 +70,7 @@ (define_split
>"TARGET_THUMB1
> && arm_disable_literal_pool
> && GET_CODE (operands[1]) == CONST_INT
> +   && !TARGET_HAVE_MOVT
> && !satisfies_constraint_I (operands[1])"
>[(clobber (const_int 0))]
>"
> @@ -696,18 +697,59 @@ (define_insn "*thumb1_movsi_insn"
>"TARGET_THUMB1
> && (   register_operand (operands[0], SImode)
> || register_operand (operands[1], SImode))"
> -  "@
> -   movs%0, %1
> -   movs%0, %1
> -   movw%0, %1
> -   #
> -   #
> -   ldmia\\t%1, {%0}
> -   stmia\\t%0, {%1}
> -   movs\\t%0, #:upper8_15:%1; lsls\\t%0, #8; adds\\t%0, #:upper0_7:%1; 
> lsls\\t%0, #8; adds\\t%0, #:lower8_15:%1; lsls\\t%0, #8; adds\\t%0, 
> #:lower0_7:%1
> -   ldr\\t%0, %1
> -   str\\t%1, %0
> -   mov\\t%0, %1"
> +{
> +  switch (which_alternative)
> +{
> +  default:
> +  case 0: return "movs\t%0, %1";
> +  case 1: return "movs\t%0, %1";
> +  case 2: return "movw\t%0, %1";
> +  case 3: return "#";
> +  case 4: return "#";
> +  case 5: return "ldmia\t%1, {%0}";
> +  case 6: return "stmia\t%0, {%1}";
> +  case 7:
> +  /* pure-code alternative: build the constant byte by byte,
> +instead of loading it from a constant pool.  */
> +   {
> + int i;
> + HOST_WIDE_INT op1 = INTVAL (operands[1]);
> + bool mov_done_p = false;
> + rtx ops[2];
> + ops[0] = operands[0];
> +
> + /* Emit upper 3 bytes if needed.  */
> + for (i = 0; i < 3; i++)
> +   {
> +  int byte = (op1 >> (8 * (3 - i))) & 0xff;
> +
> + if (byte)
> +   {
> + ops[1] = GEN_INT (byte);
> + if (mov_done_p)
> +   output_asm_insn ("adds\t%0, %1", ops);
> + else
> +   output_asm_insn ("movs\t%0, %1", ops);
> + mov_done_p = true;
> +   }
> +
> + if (mov_done_p)
> +   output_asm_insn ("lsls\t%0, #8", ops);
> +   }
> +
> + /* Emit lower byte if needed.  */
> + ops[1] = GEN_INT (op1 & 0xff);
> + if (!mov_done_p)
> +   output_asm_insn ("movs\t%0, %1", ops);
> + else if (op1 & 0xff)
> +   output_asm_insn ("adds\t%0, %1", ops);
> + return "";
> +   }
> +  case 8: return "ldr\t%0, %1";
> +  case 9: return "str\t%1, %0";
> +  case 10: return "mov\t%0, %1";
> +}
> +}
>[(set_attr "length" "2,2,4,4,4,2,2,14,2,2,2")
> (set_attr "type" 
> "mov_reg,mov_imm,mov_imm,multiple,multiple,load_4,store_4,alu_sreg,load_4,store_4,mov_reg")
> (set_attr "pool_range" "*,*,*,*,*,*,*, *,1018,*,*")
> diff --git a/gcc/testsuite/gcc.target/arm/pure-code/pr94538-1.c 
> b/gcc/testsuite/gcc.target/arm/pure-code/pr94538-1.c
> new file mode 100644
> index 000..31061d5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/pure-code/pr94538-1.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-skip-if "skip override" { *-*-* } { "-mfloat-abi=hard" } { "" } } */
> +/* { dg-options "-mpure-code -mcpu=cortex-m23 -march=armv8-m.base -mthumb 
> -mfloat-abi=soft" } */
> +
> +typedef int