| Message ID | nycvar.YFH.7.76.1909191729300.5566@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
| Series | [x86] Fix PR91814 | expand |
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)));
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)));
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);
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);
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);
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);
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)));