[x86] Fix PR91814

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.
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. | #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. | #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. | #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. | #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. | #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. | #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

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)));