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

Message ID 20210401165407.903729-1-hjl.tools@gmail.com
State New
Headers show
Series
  • x86: Don't generate uiret with -mcmodel=kernel
Related show

Commit Message

Patrick Palka via Gcc-patches April 1, 2021, 4:54 p.m.
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

-- 
2.30.2

Comments

Patrick Palka via Gcc-patches April 1, 2021, 6:31 p.m. | #1
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,
Patrick Palka via Gcc-patches April 1, 2021, 6:45 p.m. | #2
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,
Patrick Palka via Gcc-patches April 1, 2021, 8:08 p.m. | #3
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

>
Jan Hubicka April 1, 2021, 8:28 p.m. | #4
> 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

> >
Patrick Palka via Gcc-patches April 1, 2021, 8:46 p.m. | #5
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.

-- 
H.J.
Patrick Palka via Gcc-patches April 1, 2021, 8:51 p.m. | #6
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.

Patch

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 } } } } */