| Message ID | 20160429154219.GC6099@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Apr 29, 2016 at 8:42 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > Hi, > > As the first part of PR70799 fix I'd like to add constants support for > DI-STV pass (which is also related to PR70763). This patch adds CONST_INT > support as insn operands and extends cost model accordingly. > > Bootstrapped and regtested on x86_64-unknown-linux-gnu{-m32}. OK for trunk? > > Thanks, > Ilya > -- > gcc/ > > 2016-04-29 Ilya Enkovich <ilya.enkovich@intel.com> > > PR target/70799 > PR target/70763 > * config/i386/i386.c (dimode_scalar_to_vector_candidate_p): Allow > integer constants. > (dimode_scalar_chain::vector_const_cost): New. > (dimode_scalar_chain::compute_convert_gain): Handle constants. > (dimode_scalar_chain::convert_op): Likewise. > (dimode_scalar_chain::convert_insn): Likewise. I think we should fix STV regression first: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70321 In 64-bit, STV is run before CSE2 pass. Can we do that for 32-bit? H.J.
On Fri, Apr 29, 2016 at 5:48 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Fri, Apr 29, 2016 at 8:42 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> Hi, >> >> As the first part of PR70799 fix I'd like to add constants support for >> DI-STV pass (which is also related to PR70763). This patch adds CONST_INT >> support as insn operands and extends cost model accordingly. >> >> Bootstrapped and regtested on x86_64-unknown-linux-gnu{-m32}. OK for trunk? >> >> Thanks, >> Ilya >> -- >> gcc/ >> >> 2016-04-29 Ilya Enkovich <ilya.enkovich@intel.com> >> >> PR target/70799 >> PR target/70763 >> * config/i386/i386.c (dimode_scalar_to_vector_candidate_p): Allow >> integer constants. >> (dimode_scalar_chain::vector_const_cost): New. >> (dimode_scalar_chain::compute_convert_gain): Handle constants. >> (dimode_scalar_chain::convert_op): Likewise. >> (dimode_scalar_chain::convert_insn): Likewise. > > I think we should fix STV regression first: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70321 > > In 64-bit, STV is run before CSE2 pass. Can we do that for 32-bit? 64bit STV only handles constants ATM, so it is not that susceptible to where the pass is inserted. However, I think that we *can* also move 32bit STV pass before CSE2 pass. If I'm not missing some details here, the combine will then combine V2DI instructions instead of DImode insns, and this should be irrelevant as far as combine is concerned. But the above patch is orthogonal to the STV introduced regression, regression depends on a pass insertion point. Uros.
On Fri, Apr 29, 2016 at 5:42 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > Hi, > > As the first part of PR70799 fix I'd like to add constants support for > DI-STV pass (which is also related to PR70763). This patch adds CONST_INT > support as insn operands and extends cost model accordingly. > > Bootstrapped and regtested on x86_64-unknown-linux-gnu{-m32}. OK for trunk? > > Thanks, > Ilya > -- > gcc/ > > 2016-04-29 Ilya Enkovich <ilya.enkovich@intel.com> > > PR target/70799 > PR target/70763 > * config/i386/i386.c (dimode_scalar_to_vector_candidate_p): Allow > integer constants. > (dimode_scalar_chain::vector_const_cost): New. > (dimode_scalar_chain::compute_convert_gain): Handle constants. > (dimode_scalar_chain::convert_op): Likewise. > (dimode_scalar_chain::convert_insn): Likewise. > > gcc/testsuite/ > > 2016-04-29 Ilya Enkovich <ilya.enkovich@intel.com> > > * gcc.target/i386/pr70799-1.c: New test. >> @@ -3639,6 +3675,22 @@ dimode_scalar_chain::convert_op (rtx *op, rtx_insn *insn) > } > *op = gen_rtx_SUBREG (V2DImode, *op, 0); > } > + else if (CONST_INT_P (*op)) > + { > + rtx cst = const0_rtx; > + rtx tmp = gen_rtx_SUBREG (V2DImode, gen_reg_rtx (DImode), 0); > + > + /* Prefer all ones vector in case of -1. */ > + if (constm1_operand (*op, GET_MODE (*op))) > + cst = *op; > + cst = gen_rtx_CONST_VECTOR (V2DImode, gen_rtvec (2, *op, cst)); It took me a while to decipher the above functionality ;) Why not just: else if (CONST_INT_P (*op)) { rtx tmp = gen_rtx_SUBREG (V2DImode, gen_reg_rtx (DImode), 0); rtx vec; /* Prefer all ones vector in case of -1. */ if (constm1_operand (*op, GET_MODE (*op))) vec = CONSTM1_RTX (V2DImode); else vec = gen_rtx_CONST_VECTOR (V2DImode, gen_rtvec (2, *op, const0_rtx)); if (!standard_sse_constant_p (vec, V2DImode)) vec = validize_mem (force_const_mem (V2DImode, vec)); emit_insn_before (gen_move_insn (tmp, vec), insn); *op = tmp; } Comparing this part to timode_scalar_chain::convert_insn, there is a NONDEBUG_INSN_P check. Do you need one in the above code? Also, TImode pass handles REG_EQUAL and REG_EQUIV notes. Does dimode pass also need this functionality? Uros.
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index 9680aaf..ff6b4bc 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -2789,7 +2789,8 @@ dimode_scalar_to_vector_candidate_p (rtx_insn *insn) return convertible_comparison_p (insn); /* We are interested in DImode promotion only. */ - if (GET_MODE (src) != DImode + if ((GET_MODE (src) != DImode + && !CONST_INT_P (src)) || GET_MODE (dst) != DImode) return false; @@ -2809,24 +2810,31 @@ dimode_scalar_to_vector_candidate_p (rtx_insn *insn) return true; case MEM: + case CONST_INT: return REG_P (dst); default: return false; } - if (!REG_P (XEXP (src, 0)) && !MEM_P (XEXP (src, 0)) + if (!REG_P (XEXP (src, 0)) + && !MEM_P (XEXP (src, 0)) + && !CONST_INT_P (XEXP (src, 0)) /* Check for andnot case. */ && (GET_CODE (src) != AND || GET_CODE (XEXP (src, 0)) != NOT || !REG_P (XEXP (XEXP (src, 0), 0)))) return false; - if (!REG_P (XEXP (src, 1)) && !MEM_P (XEXP (src, 1))) + if (!REG_P (XEXP (src, 1)) + && !MEM_P (XEXP (src, 1)) + && !CONST_INT_P (XEXP (src, 1))) return false; - if (GET_MODE (XEXP (src, 0)) != DImode - || GET_MODE (XEXP (src, 1)) != DImode) + if ((GET_MODE (XEXP (src, 0)) != DImode + && !CONST_INT_P (XEXP (src, 0))) + || (GET_MODE (XEXP (src, 1)) != DImode + && !CONST_INT_P (XEXP (src, 1)))) return false; return true; @@ -3120,6 +3128,7 @@ class dimode_scalar_chain : public scalar_chain void convert_reg (unsigned regno); void make_vector_copies (unsigned regno); void convert_registers (); + int vector_const_cost (rtx exp); }; class timode_scalar_chain : public scalar_chain @@ -3328,6 +3337,19 @@ scalar_chain::build (bitmap candidates, unsigned insn_uid) BITMAP_FREE (queue); } +/* Return a cost of building a vector costant + instead of using a scalar one. */ + +int +dimode_scalar_chain::vector_const_cost (rtx exp) +{ + gcc_assert (CONST_INT_P (exp)); + + if (standard_sse_constant_p (exp, V2DImode)) + return COSTS_N_INSNS (1); + return ix86_cost->sse_load[1]; +} + /* Compute a gain for chain conversion. */ int @@ -3359,11 +3381,25 @@ dimode_scalar_chain::compute_convert_gain () || GET_CODE (src) == IOR || GET_CODE (src) == XOR || GET_CODE (src) == AND) - gain += ix86_cost->add; + { + gain += ix86_cost->add; + if (CONST_INT_P (XEXP (src, 0))) + gain -= vector_const_cost (XEXP (src, 0)); + if (CONST_INT_P (XEXP (src, 1))) + gain -= vector_const_cost (XEXP (src, 1)); + } else if (GET_CODE (src) == COMPARE) { /* Assume comparison cost is the same. */ } + else if (GET_CODE (src) == CONST_INT) + { + if (REG_P (dst)) + gain += COSTS_N_INSNS (2); + else if (MEM_P (dst)) + gain += 2 * ix86_cost->int_store[2] - ix86_cost->sse_store[1]; + gain -= vector_const_cost (src); + } else gcc_unreachable (); } @@ -3639,6 +3675,22 @@ dimode_scalar_chain::convert_op (rtx *op, rtx_insn *insn) } *op = gen_rtx_SUBREG (V2DImode, *op, 0); } + else if (CONST_INT_P (*op)) + { + rtx cst = const0_rtx; + rtx tmp = gen_rtx_SUBREG (V2DImode, gen_reg_rtx (DImode), 0); + + /* Prefer all ones vector in case of -1. */ + if (constm1_operand (*op, GET_MODE (*op))) + cst = *op; + cst = gen_rtx_CONST_VECTOR (V2DImode, gen_rtvec (2, *op, cst)); + + if (!standard_sse_constant_p (cst, V2DImode)) + cst = validize_mem (force_const_mem (V2DImode, cst)); + + emit_insn_before (gen_move_insn (tmp, cst), insn); + *op = tmp; + } else { gcc_assert (SUBREG_P (*op)); @@ -3711,6 +3763,10 @@ dimode_scalar_chain::convert_insn (rtx_insn *insn) UNSPEC_PTEST); break; + case CONST_INT: + convert_op (&src, insn); + break; + default: gcc_unreachable (); } diff --git a/gcc/testsuite/gcc.target/i386/pr70799-1.c b/gcc/testsuite/gcc.target/i386/pr70799-1.c new file mode 100644 index 0000000..0abbfb9 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr70799-1.c @@ -0,0 +1,41 @@ +/* PR target/pr70799 */ +/* { dg-do compile { target { ia32 } } } */ +/* { dg-options "-O2 -march=slm" } */ +/* { dg-final { scan-assembler "pxor" } } */ +/* { dg-final { scan-assembler "pcmpeqd" } } */ +/* { dg-final { scan-assembler "movdqa\[ \\t\]+.LC0" } } */ + +long long a, b, c; + +void test1 (void) +{ + long long t; + if (a) + t = 0LL; + else + t = b; + a = c & t; + b = c | t; +} + +void test2 (void) +{ + long long t; + if (a) + t = -1LL; + else + t = b; + a = c & t; + b = c | t; +} + +void test3 (void) +{ + long long t; + if (a) + t = 0xf0f0f0f0f0f0f0f0LL; + else + t = b; + a = c & t; + b = c | t; +}
Hi, As the first part of PR70799 fix I'd like to add constants support for DI-STV pass (which is also related to PR70763). This patch adds CONST_INT support as insn operands and extends cost model accordingly. Bootstrapped and regtested on x86_64-unknown-linux-gnu{-m32}. OK for trunk? Thanks, Ilya -- gcc/ 2016-04-29 Ilya Enkovich <ilya.enkovich@intel.com> PR target/70799 PR target/70763 * config/i386/i386.c (dimode_scalar_to_vector_candidate_p): Allow integer constants. (dimode_scalar_chain::vector_const_cost): New. (dimode_scalar_chain::compute_convert_gain): Handle constants. (dimode_scalar_chain::convert_op): Likewise. (dimode_scalar_chain::convert_insn): Likewise. gcc/testsuite/ 2016-04-29 Ilya Enkovich <ilya.enkovich@intel.com> * gcc.target/i386/pr70799-1.c: New test.