[x86] Fix PR91814
diff mbox series

Message ID nycvar.YFH.7.76.1909191729300.5566@zhemvz.fhfr.qr
State New
Headers show
Series
  • [x86] Fix PR91814
Related show

Commit Message

Richard Biener Sept. 19, 2019, 3:30 p.m. UTC
Boostrapped and tested on x86_64-unknown-linux-gnu.

OK?

Thanks,
Richard.

2019-09-19  Richard Biener  <rguenther@suse.de>

	PR target/91814
	* config/i386/i386-features.c (gen_gpr_to_xmm_move_src):
	Force operand to a register if it isn't nonimmediate_operand.

Comments

Uros Bizjak Sept. 19, 2019, 3:43 p.m. UTC | #1
On Thu, Sep 19, 2019 at 5:30 PM Richard Biener <rguenther@suse.de> wrote:
>
>
> Boostrapped and tested on x86_64-unknown-linux-gnu.
>
> OK?

OK.

Thanks,
Uros.

> Thanks,
> Richard.
>
> 2019-09-19  Richard Biener  <rguenther@suse.de>
>
>         PR target/91814
>         * config/i386/i386-features.c (gen_gpr_to_xmm_move_src):
>         Force operand to a register if it isn't nonimmediate_operand.
>
> Index: gcc/config/i386/i386-features.c
> ===================================================================
> --- gcc/config/i386/i386-features.c     (revision 275959)
> +++ gcc/config/i386/i386-features.c     (working copy)
> @@ -668,10 +668,13 @@ scalar_chain::emit_conversion_insns (rtx
>  static rtx
>  gen_gpr_to_xmm_move_src (enum machine_mode vmode, rtx gpr)
>  {
> +  if (!nonimmediate_operand (gpr, GET_MODE_INNER (vmode)))
> +    gpr = force_reg (GET_MODE_INNER (vmode), gpr);
>    switch (GET_MODE_NUNITS (vmode))
>      {
>      case 1:
> -      return gen_rtx_SUBREG (vmode, gpr, 0);
> +      /* We are not using this case currently.  */
> +      gcc_unreachable ();
>      case 2:
>        return gen_rtx_VEC_CONCAT (vmode, gpr,
>                                  CONST0_RTX (GET_MODE_INNER (vmode)));
Uros Bizjak Sept. 20, 2019, 8:12 a.m. UTC | #2
On Thu, Sep 19, 2019 at 5:43 PM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> On Thu, Sep 19, 2019 at 5:30 PM Richard Biener <rguenther@suse.de> wrote:
> >
> >
> > Boostrapped and tested on x86_64-unknown-linux-gnu.
> >
> > OK?
>
> OK.

Hm, something is not working correctly here. For the testcase, I get:

main:
        vmovq   %xmm0, %xmm0
        vpxor   %xmm1, %xmm1, %xmm1
        vpsubq  %xmm0, %xmm1, %xmm1
        vpmaxsq %xmm1, %xmm0, %xmm0
        vmovq   %xmm0, %rax
        movabsq %rax, .LC0+11532131096
        xorl    %eax, %eax
        ret

The first insn uses uninitialized reg.

The _.stv pass misses initialization of r94 reg:

(note 2 3 7 2 NOTE_INSN_FUNCTION_BEG)
(note 7 2 24 2 NOTE_INSN_DELETED)
(insn 24 7 8 2 (set (subreg:V2DI (reg:DI 93) 0)
        (vec_concat:V2DI (reg:DI 94)
            (const_int 0 [0]))) "pr67271.c":11:17 -1
     (nil))

Uros.

> Thanks,
> Uros.
>
> > Thanks,
> > Richard.
> >
> > 2019-09-19  Richard Biener  <rguenther@suse.de>
> >
> >         PR target/91814
> >         * config/i386/i386-features.c (gen_gpr_to_xmm_move_src):
> >         Force operand to a register if it isn't nonimmediate_operand.
> >
> > Index: gcc/config/i386/i386-features.c
> > ===================================================================
> > --- gcc/config/i386/i386-features.c     (revision 275959)
> > +++ gcc/config/i386/i386-features.c     (working copy)
> > @@ -668,10 +668,13 @@ scalar_chain::emit_conversion_insns (rtx
> >  static rtx
> >  gen_gpr_to_xmm_move_src (enum machine_mode vmode, rtx gpr)
> >  {
> > +  if (!nonimmediate_operand (gpr, GET_MODE_INNER (vmode)))
> > +    gpr = force_reg (GET_MODE_INNER (vmode), gpr);
> >    switch (GET_MODE_NUNITS (vmode))
> >      {
> >      case 1:
> > -      return gen_rtx_SUBREG (vmode, gpr, 0);
> > +      /* We are not using this case currently.  */
> > +      gcc_unreachable ();
> >      case 2:
> >        return gen_rtx_VEC_CONCAT (vmode, gpr,
> >                                  CONST0_RTX (GET_MODE_INNER (vmode)));
Richard Biener Sept. 20, 2019, 8:32 a.m. UTC | #3
On Fri, 20 Sep 2019, Uros Bizjak wrote:

> On Thu, Sep 19, 2019 at 5:43 PM Uros Bizjak <ubizjak@gmail.com> wrote:
> >
> > On Thu, Sep 19, 2019 at 5:30 PM Richard Biener <rguenther@suse.de> wrote:
> > >
> > >
> > > Boostrapped and tested on x86_64-unknown-linux-gnu.
> > >
> > > OK?
> >
> > OK.
> 
> Hm, something is not working correctly here. For the testcase, I get:
> 
> main:
>         vmovq   %xmm0, %xmm0
>         vpxor   %xmm1, %xmm1, %xmm1
>         vpsubq  %xmm0, %xmm1, %xmm1
>         vpmaxsq %xmm1, %xmm0, %xmm0
>         vmovq   %xmm0, %rax
>         movabsq %rax, .LC0+11532131096
>         xorl    %eax, %eax
>         ret
> 
> The first insn uses uninitialized reg.
> 
> The _.stv pass misses initialization of r94 reg:
> 
> (note 2 3 7 2 NOTE_INSN_FUNCTION_BEG)
> (note 7 2 24 2 NOTE_INSN_DELETED)
> (insn 24 7 8 2 (set (subreg:V2DI (reg:DI 93) 0)
>         (vec_concat:V2DI (reg:DI 94)
>             (const_int 0 [0]))) "pr67271.c":11:17 -1
>      (nil))

Hmm, it emits the instruction in the wrong spot for the caller

      emit_insn_before (gen_rtx_SET (gen_rtx_SUBREG (vmode, tmp, 0),
                                     gen_gpr_to_xmm_move_src (vmode, 
*op)),
                        insn);

we can do the following which I am testing now.

Richard.

2019-09-20  Richard Biener  <rguenther@suse.de>

	PR target/91814
	* config/i386/i386-features.c (gen_gpr_to_xmm_move_src): Add
	before parameter to indicate where to emit a needed move to.
	(general_scalar_chain::convert_op): Use it.

Index: gcc/config/i386/i386-features.c
===================================================================
--- gcc/config/i386/i386-features.c	(revision 275989)
+++ gcc/config/i386/i386-features.c	(working copy)
@@ -666,18 +666,26 @@ scalar_chain::emit_conversion_insns (rtx
    zeroing the upper parts.  */
 
 static rtx
-gen_gpr_to_xmm_move_src (enum machine_mode vmode, rtx gpr)
+gen_gpr_to_xmm_move_src (enum machine_mode vmode, rtx gpr,
+			 rtx_insn *before = NULL)
 {
-  if (!nonimmediate_operand (gpr, GET_MODE_INNER (vmode)))
-    gpr = force_reg (GET_MODE_INNER (vmode), gpr);
+  enum machine_mode smode = GET_MODE_INNER (vmode);
+  if (!nonimmediate_operand (gpr, smode))
+    {
+      rtx tem = gen_reg_rtx (smode);
+      if (before)
+	emit_insn_before (gen_rtx_SET (tem, gpr), before);
+      else
+	emit_move_insn (tem, gpr);
+      gpr = tem;
+    }
   switch (GET_MODE_NUNITS (vmode))
     {
     case 1:
       /* We are not using this case currently.  */
       gcc_unreachable ();
     case 2:
-      return gen_rtx_VEC_CONCAT (vmode, gpr,
-				 CONST0_RTX (GET_MODE_INNER (vmode)));
+      return gen_rtx_VEC_CONCAT (vmode, gpr, CONST0_RTX (smode));
     default:
       return gen_rtx_VEC_MERGE (vmode, gen_rtx_VEC_DUPLICATE (vmode, gpr),
 				CONST0_RTX (vmode), GEN_INT (HOST_WIDE_INT_1U));
@@ -836,7 +844,8 @@ general_scalar_chain::convert_op (rtx *o
       rtx tmp = gen_reg_rtx (GET_MODE (*op));
 
       emit_insn_before (gen_rtx_SET (gen_rtx_SUBREG (vmode, tmp, 0),
-				     gen_gpr_to_xmm_move_src (vmode, *op)),
+				     gen_gpr_to_xmm_move_src (vmode, *op,
+							      insn)),
 			insn);
       *op = gen_rtx_SUBREG (vmode, tmp, 0);
Uros Bizjak Sept. 20, 2019, 9:09 a.m. UTC | #4
On Fri, Sep 20, 2019 at 10:32 AM Richard Biener <rguenther@suse.de> wrote:
>
> On Fri, 20 Sep 2019, Uros Bizjak wrote:
>
> > On Thu, Sep 19, 2019 at 5:43 PM Uros Bizjak <ubizjak@gmail.com> wrote:
> > >
> > > On Thu, Sep 19, 2019 at 5:30 PM Richard Biener <rguenther@suse.de> wrote:
> > > >
> > > >
> > > > Boostrapped and tested on x86_64-unknown-linux-gnu.
> > > >
> > > > OK?
> > >
> > > OK.
> >
> > Hm, something is not working correctly here. For the testcase, I get:
> >
> > main:
> >         vmovq   %xmm0, %xmm0
> >         vpxor   %xmm1, %xmm1, %xmm1
> >         vpsubq  %xmm0, %xmm1, %xmm1
> >         vpmaxsq %xmm1, %xmm0, %xmm0
> >         vmovq   %xmm0, %rax
> >         movabsq %rax, .LC0+11532131096
> >         xorl    %eax, %eax
> >         ret
> >
> > The first insn uses uninitialized reg.
> >
> > The _.stv pass misses initialization of r94 reg:
> >
> > (note 2 3 7 2 NOTE_INSN_FUNCTION_BEG)
> > (note 7 2 24 2 NOTE_INSN_DELETED)
> > (insn 24 7 8 2 (set (subreg:V2DI (reg:DI 93) 0)
> >         (vec_concat:V2DI (reg:DI 94)
> >             (const_int 0 [0]))) "pr67271.c":11:17 -1
> >      (nil))
>
> Hmm, it emits the instruction in the wrong spot for the caller
>
>       emit_insn_before (gen_rtx_SET (gen_rtx_SUBREG (vmode, tmp, 0),
>                                      gen_gpr_to_xmm_move_src (vmode,
> *op)),
>                         insn);
>
> we can do the following which I am testing now.

How about the attached patch? The only place we process memory operand
is from convert_op (see the function comment), and by using
gen_rtx_SET directly, we can still generate MOVABS, which is otherwise
split by using emit_move_insn.

Can you please test the attached patch instead?

Thanks,
Uros.
diff --git a/gcc/config/i386/i386-features.c b/gcc/config/i386/i386-features.c
index 546d78d99b53..9b297bac1910 100644
--- a/gcc/config/i386/i386-features.c
+++ b/gcc/config/i386/i386-features.c
@@ -668,8 +668,6 @@ scalar_chain::emit_conversion_insns (rtx insns, rtx_insn *after)
 static rtx
 gen_gpr_to_xmm_move_src (enum machine_mode vmode, rtx gpr)
 {
-  if (!nonimmediate_operand (gpr, GET_MODE_INNER (vmode)))
-    gpr = force_reg (GET_MODE_INNER (vmode), gpr);
   switch (GET_MODE_NUNITS (vmode))
     {
     case 1:
@@ -835,6 +833,15 @@ general_scalar_chain::convert_op (rtx *op, rtx_insn *insn)
     {
       rtx tmp = gen_reg_rtx (GET_MODE (*op));
 
+      /* Handle movabs.  */
+      if (!memory_operand (*op, GET_MODE (*op)))
+	{
+	  rtx tmp2 = gen_reg_rtx (GET_MODE (*op));
+
+	  emit_insn_before (gen_rtx_SET (tmp2, *op), insn);
+	  *op = tmp2;
+	}
+
       emit_insn_before (gen_rtx_SET (gen_rtx_SUBREG (vmode, tmp, 0),
 				     gen_gpr_to_xmm_move_src (vmode, *op)),
 			insn);
Richard Biener Sept. 20, 2019, 11:11 a.m. UTC | #5
On Fri, 20 Sep 2019, Uros Bizjak wrote:

> On Fri, Sep 20, 2019 at 10:32 AM Richard Biener <rguenther@suse.de> wrote:
> >
> > On Fri, 20 Sep 2019, Uros Bizjak wrote:
> >
> > > On Thu, Sep 19, 2019 at 5:43 PM Uros Bizjak <ubizjak@gmail.com> wrote:
> > > >
> > > > On Thu, Sep 19, 2019 at 5:30 PM Richard Biener <rguenther@suse.de> wrote:
> > > > >
> > > > >
> > > > > Boostrapped and tested on x86_64-unknown-linux-gnu.
> > > > >
> > > > > OK?
> > > >
> > > > OK.
> > >
> > > Hm, something is not working correctly here. For the testcase, I get:
> > >
> > > main:
> > >         vmovq   %xmm0, %xmm0
> > >         vpxor   %xmm1, %xmm1, %xmm1
> > >         vpsubq  %xmm0, %xmm1, %xmm1
> > >         vpmaxsq %xmm1, %xmm0, %xmm0
> > >         vmovq   %xmm0, %rax
> > >         movabsq %rax, .LC0+11532131096
> > >         xorl    %eax, %eax
> > >         ret
> > >
> > > The first insn uses uninitialized reg.
> > >
> > > The _.stv pass misses initialization of r94 reg:
> > >
> > > (note 2 3 7 2 NOTE_INSN_FUNCTION_BEG)
> > > (note 7 2 24 2 NOTE_INSN_DELETED)
> > > (insn 24 7 8 2 (set (subreg:V2DI (reg:DI 93) 0)
> > >         (vec_concat:V2DI (reg:DI 94)
> > >             (const_int 0 [0]))) "pr67271.c":11:17 -1
> > >      (nil))
> >
> > Hmm, it emits the instruction in the wrong spot for the caller
> >
> >       emit_insn_before (gen_rtx_SET (gen_rtx_SUBREG (vmode, tmp, 0),
> >                                      gen_gpr_to_xmm_move_src (vmode,
> > *op)),
> >                         insn);
> >
> > we can do the following which I am testing now.
> 
> How about the attached patch? The only place we process memory operand
> is from convert_op (see the function comment), and by using
> gen_rtx_SET directly, we can still generate MOVABS, which is otherwise
> split by using emit_move_insn.
> 
> Can you please test the attached patch instead?

Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?

Thanks,
Richard.

2019-09-20  Richard Biener  <rguenther@suse.de>
	Uros Bizjak  <ubizjak@gmail.com>

	PR target/91814
	* config/i386/i386-features.c (gen_gpr_to_xmm_move_src): Revert
	previous change.
	(general_scalar_chain::convert_op): Force not suitable memory
	operands to a register.

Index: gcc/config/i386/i386-features.c
===================================================================
--- gcc/config/i386/i386-features.c	(revision 275995)
+++ gcc/config/i386/i386-features.c	(working copy)
@@ -668,8 +668,6 @@ scalar_chain::emit_conversion_insns (rtx
 static rtx
 gen_gpr_to_xmm_move_src (enum machine_mode vmode, rtx gpr)
 {
-  if (!nonimmediate_operand (gpr, GET_MODE_INNER (vmode)))
-    gpr = force_reg (GET_MODE_INNER (vmode), gpr);
   switch (GET_MODE_NUNITS (vmode))
     {
     case 1:
@@ -835,6 +833,15 @@ general_scalar_chain::convert_op (rtx *o
     {
       rtx tmp = gen_reg_rtx (GET_MODE (*op));
 
+      /* Handle movabs.  */
+      if (!memory_operand (*op, GET_MODE (*op)))
+	{
+	  rtx tmp2 = gen_reg_rtx (GET_MODE (*op));
+
+	  emit_insn_before (gen_rtx_SET (tmp2, *op), insn);
+	  *op = tmp2;
+	}
+
       emit_insn_before (gen_rtx_SET (gen_rtx_SUBREG (vmode, tmp, 0),
 				     gen_gpr_to_xmm_move_src (vmode, *op)),
 			insn);
Uros Bizjak Sept. 20, 2019, 11:13 a.m. UTC | #6
On Fri, Sep 20, 2019 at 1:11 PM Richard Biener <rguenther@suse.de> wrote:
>
> On Fri, 20 Sep 2019, Uros Bizjak wrote:
>
> > On Fri, Sep 20, 2019 at 10:32 AM Richard Biener <rguenther@suse.de> wrote:
> > >
> > > On Fri, 20 Sep 2019, Uros Bizjak wrote:
> > >
> > > > On Thu, Sep 19, 2019 at 5:43 PM Uros Bizjak <ubizjak@gmail.com> wrote:
> > > > >
> > > > > On Thu, Sep 19, 2019 at 5:30 PM Richard Biener <rguenther@suse.de> wrote:
> > > > > >
> > > > > >
> > > > > > Boostrapped and tested on x86_64-unknown-linux-gnu.
> > > > > >
> > > > > > OK?
> > > > >
> > > > > OK.
> > > >
> > > > Hm, something is not working correctly here. For the testcase, I get:
> > > >
> > > > main:
> > > >         vmovq   %xmm0, %xmm0
> > > >         vpxor   %xmm1, %xmm1, %xmm1
> > > >         vpsubq  %xmm0, %xmm1, %xmm1
> > > >         vpmaxsq %xmm1, %xmm0, %xmm0
> > > >         vmovq   %xmm0, %rax
> > > >         movabsq %rax, .LC0+11532131096
> > > >         xorl    %eax, %eax
> > > >         ret
> > > >
> > > > The first insn uses uninitialized reg.
> > > >
> > > > The _.stv pass misses initialization of r94 reg:
> > > >
> > > > (note 2 3 7 2 NOTE_INSN_FUNCTION_BEG)
> > > > (note 7 2 24 2 NOTE_INSN_DELETED)
> > > > (insn 24 7 8 2 (set (subreg:V2DI (reg:DI 93) 0)
> > > >         (vec_concat:V2DI (reg:DI 94)
> > > >             (const_int 0 [0]))) "pr67271.c":11:17 -1
> > > >      (nil))
> > >
> > > Hmm, it emits the instruction in the wrong spot for the caller
> > >
> > >       emit_insn_before (gen_rtx_SET (gen_rtx_SUBREG (vmode, tmp, 0),
> > >                                      gen_gpr_to_xmm_move_src (vmode,
> > > *op)),
> > >                         insn);
> > >
> > > we can do the following which I am testing now.
> >
> > How about the attached patch? The only place we process memory operand
> > is from convert_op (see the function comment), and by using
> > gen_rtx_SET directly, we can still generate MOVABS, which is otherwise
> > split by using emit_move_insn.
> >
> > Can you please test the attached patch instead?
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?
>
> Thanks,
> Richard.
>
> 2019-09-20  Richard Biener  <rguenther@suse.de>
>         Uros Bizjak  <ubizjak@gmail.com>
>
>         PR target/91814
>         * config/i386/i386-features.c (gen_gpr_to_xmm_move_src): Revert
>         previous change.
>         (general_scalar_chain::convert_op): Force not suitable memory
>         operands to a register.

OK.

Thanks,
Uros.

> Index: gcc/config/i386/i386-features.c
> ===================================================================
> --- gcc/config/i386/i386-features.c     (revision 275995)
> +++ gcc/config/i386/i386-features.c     (working copy)
> @@ -668,8 +668,6 @@ scalar_chain::emit_conversion_insns (rtx
>  static rtx
>  gen_gpr_to_xmm_move_src (enum machine_mode vmode, rtx gpr)
>  {
> -  if (!nonimmediate_operand (gpr, GET_MODE_INNER (vmode)))
> -    gpr = force_reg (GET_MODE_INNER (vmode), gpr);
>    switch (GET_MODE_NUNITS (vmode))
>      {
>      case 1:
> @@ -835,6 +833,15 @@ general_scalar_chain::convert_op (rtx *o
>      {
>        rtx tmp = gen_reg_rtx (GET_MODE (*op));
>
> +      /* Handle movabs.  */
> +      if (!memory_operand (*op, GET_MODE (*op)))
> +       {
> +         rtx tmp2 = gen_reg_rtx (GET_MODE (*op));
> +
> +         emit_insn_before (gen_rtx_SET (tmp2, *op), insn);
> +         *op = tmp2;
> +       }
> +
>        emit_insn_before (gen_rtx_SET (gen_rtx_SUBREG (vmode, tmp, 0),
>                                      gen_gpr_to_xmm_move_src (vmode, *op)),
>                         insn);

Patch
diff mbox series

Index: gcc/config/i386/i386-features.c
===================================================================
--- gcc/config/i386/i386-features.c	(revision 275959)
+++ gcc/config/i386/i386-features.c	(working copy)
@@ -668,10 +668,13 @@  scalar_chain::emit_conversion_insns (rtx
 static rtx
 gen_gpr_to_xmm_move_src (enum machine_mode vmode, rtx gpr)
 {
+  if (!nonimmediate_operand (gpr, GET_MODE_INNER (vmode)))
+    gpr = force_reg (GET_MODE_INNER (vmode), gpr);
   switch (GET_MODE_NUNITS (vmode))
     {
     case 1:
-      return gen_rtx_SUBREG (vmode, gpr, 0);
+      /* We are not using this case currently.  */
+      gcc_unreachable ();
     case 2:
       return gen_rtx_VEC_CONCAT (vmode, gpr,
 				 CONST0_RTX (GET_MODE_INNER (vmode)));