x86: Don't generate uiret with -mcmodel=kernel

Since uiret should be used only for user interrupt handler return, don't
generate uiret in interrupt handler return with -mcmodel=kernel even if
UINTR is enabled.

gcc/

	PR target/99870
	* config/i386/i386.md (interrupt_return): Don't generate uiret
	for -mcmodel=kernel.

gcc/testsuite/

	* gcc.target/i386/pr99870.c: New test.
---
 gcc/config/i386/i386.md                 |  3 ++-
 gcc/testsuite/gcc.target/i386/pr99870.c | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr99870.c

On 1 April 2021 18:54:07 CEST, "H.J. Lu via Gcc-patches" <gcc-patches@gcc.gnu.org> wrote:
>Since uiret should be used only for user interrupt handler return,
>don't
>generate uiret in interrupt handler return with -mcmodel=kernel even if
>UINTR is enabled.
>
>gcc/
>
>	PR target/99870
>	* config/i386/i386.md (interrupt_return): Don't generate uiret
>	for -mcmodel=kernel.
>
>gcc/testsuite/
>
>	* gcc.target/i386/pr99870.c: New test.
>---
> gcc/config/i386/i386.md                 |  3 ++-
> gcc/testsuite/gcc.target/i386/pr99870.c | 19 +++++++++++++++++++
> 2 files changed, 21 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.target/i386/pr99870.c
>
>diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
>index 9ff35d9a607..1055b4187b2 100644
>--- a/gcc/config/i386/i386.md
>+++ b/gcc/config/i386/i386.md
>@@ -13885,7 +13885,8 @@ (define_insn "interrupt_return"
>    (unspec [(const_int 0)] UNSPEC_INTERRUPT_RETURN)]
>   "reload_completed"
> {
>-  return TARGET_64BIT ? (TARGET_UINTR ? "uiret" : "iretq") : "iret";
>+  return TARGET_64BIT ? ((TARGET_UINTR && ix86_cmodel != CM_KERNEL)
>+			 ? "uiret" : "iretq") : "iret";
> })
> 
>;; Used by x86_machine_dependent_reorg to avoid penalty on single byte
>RET
>diff --git a/gcc/testsuite/gcc.target/i386/pr99870.c
>b/gcc/testsuite/gcc.target/i386/pr99870.c
>new file mode 100644
>index 00000000000..0dafa001ba9
>--- /dev/null
>+++ b/gcc/testsuite/gcc.target/i386/pr99870.c
>@@ -0,0 +1,19 @@
>+/* { dg-do compile } */
>+/* { dg-options "-O2 -march=sapphirerapids -mgeneral-regs-only" } */
>+/* { dg-additional-options "-mcmodel=kernel" { target { ! ia32 } } }
>*/
>+
>+void
>+__attribute__((interrupt))
>+fn (void *frame)
>+{
>+}
>+
>+typedef void (*fn_t) (void *) __attribute__((interrupt));
>+
>+fn_t fns[] =
>+{
>+  fn,
>+};
>+
>+/* { dg-final { scan-assembler-times "\\tiret" 1 { target ia32 } } }

This matches ia32 and the others as well (x32 and amd64 or x86_64) doesn't it.
>*/
>+/* { dg-final { scan-assembler-times "\\tiretq" 1 { target { ! ia32 }
>} } } */

So this is superfluous without a trailing anchor for the first, fwiw.
Thanks,

On 1 April 2021 20:31:13 CEST, Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:
>On 1 April 2021 18:54:07 CEST, "H.J. Lu via Gcc-patches"
><gcc-patches@gcc.gnu.org> wrote:
>>Since uiret should be used only for user interrupt handler return,
>>don't
>>generate uiret in interrupt handler return with -mcmodel=kernel even
>if
>>UINTR is enabled.
>>
>>gcc/
>>
>>	PR target/99870
>>	* config/i386/i386.md (interrupt_return): Don't generate uiret
>>	for -mcmodel=kernel.
>>
>>gcc/testsuite/
>>
>>	* gcc.target/i386/pr99870.c: New test.
>>---
>> gcc/config/i386/i386.md                 |  3 ++-
>> gcc/testsuite/gcc.target/i386/pr99870.c | 19 +++++++++++++++++++
>> 2 files changed, 21 insertions(+), 1 deletion(-)
>> create mode 100644 gcc/testsuite/gcc.target/i386/pr99870.c
>>
>>diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
>>index 9ff35d9a607..1055b4187b2 100644
>>--- a/gcc/config/i386/i386.md
>>+++ b/gcc/config/i386/i386.md
>>@@ -13885,7 +13885,8 @@ (define_insn "interrupt_return"
>>    (unspec [(const_int 0)] UNSPEC_INTERRUPT_RETURN)]
>>   "reload_completed"
>> {
>>-  return TARGET_64BIT ? (TARGET_UINTR ? "uiret" : "iretq") : "iret";
>>+  return TARGET_64BIT ? ((TARGET_UINTR && ix86_cmodel != CM_KERNEL)
>>+			 ? "uiret" : "iretq") : "iret";
>> })
>> 
>>;; Used by x86_machine_dependent_reorg to avoid penalty on single byte
>>RET
>>diff --git a/gcc/testsuite/gcc.target/i386/pr99870.c
>>b/gcc/testsuite/gcc.target/i386/pr99870.c
>>new file mode 100644
>>index 00000000000..0dafa001ba9
>>--- /dev/null
>>+++ b/gcc/testsuite/gcc.target/i386/pr99870.c
>>@@ -0,0 +1,19 @@
>>+/* { dg-do compile } */
>>+/* { dg-options "-O2 -march=sapphirerapids -mgeneral-regs-only" } */
>>+/* { dg-additional-options "-mcmodel=kernel" { target { ! ia32 } } }
>>*/
>>+
>>+void
>>+__attribute__((interrupt))
>>+fn (void *frame)

Oh and of course that named param should trigger an unused parm warning making the test fail as is.
Why wasn't that flagged? Isn't this warning on by default?

>>+{
>>+}
>>+
>>+typedef void (*fn_t) (void *) __attribute__((interrupt));
>>+
>>+fn_t fns[] =
>>+{
>>+  fn,
>>+};
>>+
>>+/* { dg-final { scan-assembler-times "\\tiret" 1 { target ia32 } } }
>
>This matches ia32 and the others as well (x32 and amd64 or x86_64)
>doesn't it.
>>*/
>>+/* { dg-final { scan-assembler-times "\\tiretq" 1 { target { ! ia32 }
>>} } } */
>
>So this is superfluous without a trailing anchor for the first, fwiw.
>Thanks,

On Thu, Apr 1, 2021 at 6:54 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> Since uiret should be used only for user interrupt handler return, don't
> generate uiret in interrupt handler return with -mcmodel=kernel even if
> UINTR is enabled.

NAK, -mcmodel should not affect ISAs, in the same way it doesn't switch off SSE.

Uros.

> gcc/
>
>         PR target/99870
>         * config/i386/i386.md (interrupt_return): Don't generate uiret
>         for -mcmodel=kernel.
>
> gcc/testsuite/
>
>         * gcc.target/i386/pr99870.c: New test.
> ---
>  gcc/config/i386/i386.md                 |  3 ++-
>  gcc/testsuite/gcc.target/i386/pr99870.c | 19 +++++++++++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr99870.c
>
> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> index 9ff35d9a607..1055b4187b2 100644
> --- a/gcc/config/i386/i386.md
> +++ b/gcc/config/i386/i386.md
> @@ -13885,7 +13885,8 @@ (define_insn "interrupt_return"
>     (unspec [(const_int 0)] UNSPEC_INTERRUPT_RETURN)]
>    "reload_completed"
>  {
> -  return TARGET_64BIT ? (TARGET_UINTR ? "uiret" : "iretq") : "iret";
> +  return TARGET_64BIT ? ((TARGET_UINTR && ix86_cmodel != CM_KERNEL)
> +                        ? "uiret" : "iretq") : "iret";
>  })
>
>  ;; Used by x86_machine_dependent_reorg to avoid penalty on single byte RET
> diff --git a/gcc/testsuite/gcc.target/i386/pr99870.c b/gcc/testsuite/gcc.target/i386/pr99870.c
> new file mode 100644
> index 00000000000..0dafa001ba9
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr99870.c
> @@ -0,0 +1,19 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -march=sapphirerapids -mgeneral-regs-only" } */
> +/* { dg-additional-options "-mcmodel=kernel" { target { ! ia32 } } } */
> +
> +void
> +__attribute__((interrupt))
> +fn (void *frame)
> +{
> +}
> +
> +typedef void (*fn_t) (void *) __attribute__((interrupt));
> +
> +fn_t fns[] =
> +{
> +  fn,
> +};
> +
> +/* { dg-final { scan-assembler-times "\\tiret" 1 { target ia32 } } } */
> +/* { dg-final { scan-assembler-times "\\tiretq" 1 { target { ! ia32 } } } } */
> --
> 2.30.2
>

> On Thu, Apr 1, 2021 at 6:54 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > Since uiret should be used only for user interrupt handler return, don't
> > generate uiret in interrupt handler return with -mcmodel=kernel even if
> > UINTR is enabled.
> 
> NAK, -mcmodel should not affect ISAs, in the same way it doesn't switch off SSE.

Agreed, while kernel name might not be bit too suggestive, code models
was intended to be quite general, so it may make sense to compile things
that are not kernels with kernel cmodel...

Honza
> 
> Uros.
> 
> > gcc/
> >
> >         PR target/99870
> >         * config/i386/i386.md (interrupt_return): Don't generate uiret
> >         for -mcmodel=kernel.
> >
> > gcc/testsuite/
> >
> >         * gcc.target/i386/pr99870.c: New test.
> > ---
> >  gcc/config/i386/i386.md                 |  3 ++-
> >  gcc/testsuite/gcc.target/i386/pr99870.c | 19 +++++++++++++++++++
> >  2 files changed, 21 insertions(+), 1 deletion(-)
> >  create mode 100644 gcc/testsuite/gcc.target/i386/pr99870.c
> >
> > diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> > index 9ff35d9a607..1055b4187b2 100644
> > --- a/gcc/config/i386/i386.md
> > +++ b/gcc/config/i386/i386.md
> > @@ -13885,7 +13885,8 @@ (define_insn "interrupt_return"
> >     (unspec [(const_int 0)] UNSPEC_INTERRUPT_RETURN)]
> >    "reload_completed"
> >  {
> > -  return TARGET_64BIT ? (TARGET_UINTR ? "uiret" : "iretq") : "iret";
> > +  return TARGET_64BIT ? ((TARGET_UINTR && ix86_cmodel != CM_KERNEL)
> > +                        ? "uiret" : "iretq") : "iret";
> >  })
> >
> >  ;; Used by x86_machine_dependent_reorg to avoid penalty on single byte RET
> > diff --git a/gcc/testsuite/gcc.target/i386/pr99870.c b/gcc/testsuite/gcc.target/i386/pr99870.c
> > new file mode 100644
> > index 00000000000..0dafa001ba9
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/i386/pr99870.c
> > @@ -0,0 +1,19 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -march=sapphirerapids -mgeneral-regs-only" } */
> > +/* { dg-additional-options "-mcmodel=kernel" { target { ! ia32 } } } */
> > +
> > +void
> > +__attribute__((interrupt))
> > +fn (void *frame)
> > +{
> > +}
> > +
> > +typedef void (*fn_t) (void *) __attribute__((interrupt));
> > +
> > +fn_t fns[] =
> > +{
> > +  fn,
> > +};
> > +
> > +/* { dg-final { scan-assembler-times "\\tiret" 1 { target ia32 } } } */
> > +/* { dg-final { scan-assembler-times "\\tiretq" 1 { target { ! ia32 } } } } */
> > --
> > 2.30.2
> >

On Thu, Apr 1, 2021 at 1:28 PM Jan Hubicka <hubicka@ucw.cz> wrote:
>
> > On Thu, Apr 1, 2021 at 6:54 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > >
> > > Since uiret should be used only for user interrupt handler return, don't
> > > generate uiret in interrupt handler return with -mcmodel=kernel even if
> > > UINTR is enabled.
> >
> > NAK, -mcmodel should not affect ISAs, in the same way it doesn't switch off SSE.
>
> Agreed, while kernel name might not be bit too suggestive, code models
> was intended to be quite general, so it may make sense to compile things
> that are not kernels with kernel cmodel...
>

Then kernel must be built with -mno-uintr if there are kernel interrupt handlers
in C.  That means that other UINTR intrinsics won't be available to
kernel source
and inline asm statement should be used instead.

On Thu, Apr 1, 2021 at 10:47 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Thu, Apr 1, 2021 at 1:28 PM Jan Hubicka <hubicka@ucw.cz> wrote:
> >
> > > On Thu, Apr 1, 2021 at 6:54 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > >
> > > > Since uiret should be used only for user interrupt handler return, don't
> > > > generate uiret in interrupt handler return with -mcmodel=kernel even if
> > > > UINTR is enabled.
> > >
> > > NAK, -mcmodel should not affect ISAs, in the same way it doesn't switch off SSE.
> >
> > Agreed, while kernel name might not be bit too suggestive, code models
> > was intended to be quite general, so it may make sense to compile things
> > that are not kernels with kernel cmodel...
> >
>
> Then kernel must be built with -mno-uintr if there are kernel interrupt handlers
> in C.  That means that other UINTR intrinsics won't be available to
> kernel source
> and inline asm statement should be used instead.

This is what the kernel does anyway. It doesn't care for compiler builtins.

Uros.