* [PATCH] middle-end/61747 - conditional move expansion and constants @ 2023-07-18 11:25 Richard Biener 2023-07-19 13:11 ` Jakub Jelinek 0 siblings, 1 reply; 4+ messages in thread From: Richard Biener @ 2023-07-18 11:25 UTC (permalink / raw) To: gcc-patches; +Cc: Jakub Jelinek When expanding a COND_EXPR or a VEC_COND_EXPR the x86 backend for example tries to match FP min/max instructions. But this only works when it can see the equality of the comparison and selected operands. This breaks in both prepare_cmp_insn and vector_compare_rtx where the former forces expensive constants to a register and the latter performs legitimization. The patch below fixes this in the caller preserving former equalities. Bootstrap and regtest in progress. OK if that succeeds? Thanks, Richard. PR middle-end/61747 * internal-fn.cc (expand_vec_cond_optab_fn): When the value operands are equal to the original comparison operands preserve that equality by re-using the comparison expansion. * optabs.cc (emit_conditional_move): When the value operands are equal to the comparison operands and would be forced to a register by prepare_cmp_insn do so earlier, preserving the equality. * g++.target/i386/pr61747.C: New testcase. --- gcc/internal-fn.cc | 17 ++++++++-- gcc/optabs.cc | 32 ++++++++++++++++++- gcc/testsuite/g++.target/i386/pr61747.C | 42 +++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 3 deletions(-) create mode 100644 gcc/testsuite/g++.target/i386/pr61747.C diff --git a/gcc/internal-fn.cc b/gcc/internal-fn.cc index e698f0bffc7..c83c3921792 100644 --- a/gcc/internal-fn.cc +++ b/gcc/internal-fn.cc @@ -3019,8 +3019,21 @@ expand_vec_cond_optab_fn (internal_fn, gcall *stmt, convert_optab optab) icode = convert_optab_handler (optab, mode, cmp_op_mode); rtx comparison = vector_compare_rtx (VOIDmode, tcode, op0a, op0b, unsignedp, icode, 4); - rtx rtx_op1 = expand_normal (op1); - rtx rtx_op2 = expand_normal (op2); + /* vector_compare_rtx legitimizes operands, preserve equality when + expanding op1/op2. */ + rtx rtx_op1, rtx_op2; + if (operand_equal_p (op1, op0a)) + rtx_op1 = XEXP (comparison, 0); + else if (operand_equal_p (op1, op0b)) + rtx_op1 = XEXP (comparison, 1); + else + rtx_op1 = expand_normal (op1); + if (operand_equal_p (op2, op0a)) + rtx_op2 = XEXP (comparison, 0); + else if (operand_equal_p (op2, op0b)) + rtx_op2 = XEXP (comparison, 1); + else + rtx_op2 = expand_normal (op2); rtx target = expand_expr (lhs, NULL_RTX, VOIDmode, EXPAND_WRITE); create_output_operand (&ops[0], target, mode); diff --git a/gcc/optabs.cc b/gcc/optabs.cc index 4e9f58f8060..a9ba3267666 100644 --- a/gcc/optabs.cc +++ b/gcc/optabs.cc @@ -5119,13 +5119,43 @@ emit_conditional_move (rtx target, struct rtx_comparison comp, last = get_last_insn (); do_pending_stack_adjust (); machine_mode cmpmode = comp.mode; + rtx orig_op0 = XEXP (comparison, 0); + rtx orig_op1 = XEXP (comparison, 1); + rtx op2p = op2; + rtx op3p = op3; + /* If we are optimizing, force expensive constants into a register + but preserve an eventual equality with op2/op3. */ + if (CONSTANT_P (orig_op0) && optimize + && (rtx_cost (orig_op0, mode, COMPARE, 0, + optimize_insn_for_speed_p ()) + > COSTS_N_INSNS (1)) + && can_create_pseudo_p ()) + { + XEXP (comparison, 0) = force_reg (cmpmode, orig_op0); + if (rtx_equal_p (orig_op0, op2)) + op2p = XEXP (comparison, 0); + if (rtx_equal_p (orig_op0, op3)) + op3p = XEXP (comparison, 0); + } + if (CONSTANT_P (orig_op1) && optimize + && (rtx_cost (orig_op1, mode, COMPARE, 0, + optimize_insn_for_speed_p ()) + > COSTS_N_INSNS (1)) + && can_create_pseudo_p ()) + { + XEXP (comparison, 1) = force_reg (cmpmode, orig_op1); + if (rtx_equal_p (orig_op1, op2)) + op2p = XEXP (comparison, 1); + if (rtx_equal_p (orig_op1, op3)) + op3p = XEXP (comparison, 1); + } prepare_cmp_insn (XEXP (comparison, 0), XEXP (comparison, 1), GET_CODE (comparison), NULL_RTX, unsignedp, OPTAB_WIDEN, &comparison, &cmpmode); if (comparison) { rtx res = emit_conditional_move_1 (target, comparison, - op2, op3, mode); + op2p, op3p, mode); if (res != NULL_RTX) return res; } diff --git a/gcc/testsuite/g++.target/i386/pr61747.C b/gcc/testsuite/g++.target/i386/pr61747.C new file mode 100644 index 00000000000..024ef400052 --- /dev/null +++ b/gcc/testsuite/g++.target/i386/pr61747.C @@ -0,0 +1,42 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target c++11 } */ +/* { dg-options "-O2 -msse4.1 -mfpmath=sse" } */ + +typedef float __attribute__( ( vector_size( 16 ) ) ) float32x4_t; + +template<typename V1> +V1 vmax(V1 a, V1 b) { + return (a>b) ? a : b; +} + +template<typename V1> +V1 vmin(V1 a, V1 b) { + return (a<b) ? a : b; +} + +float foo(float a, float b, float c) { + return vmin(vmax(a,b),c); +} + +float32x4_t foo(float32x4_t a, float32x4_t b, float32x4_t c) { + return vmin(vmax(a,b),c); +} + +template<typename Float> +Float bart(Float a) { + constexpr Float zero{0.f}; + constexpr Float it = zero+4.f; + constexpr Float zt = zero-3.f; + return vmin(vmax(a,zt),it); +} + +float bar(float a) { + return bart(a); +} + +float32x4_t bar(float32x4_t a) { + return bart(a); +} + +/* { dg-final { scan-assembler-times "min" 4 } } */ +/* { dg-final { scan-assembler-times "max" 4 } } */ -- 2.35.3 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] middle-end/61747 - conditional move expansion and constants 2023-07-18 11:25 [PATCH] middle-end/61747 - conditional move expansion and constants Richard Biener @ 2023-07-19 13:11 ` Jakub Jelinek 2023-07-19 13:36 ` Richard Biener 0 siblings, 1 reply; 4+ messages in thread From: Jakub Jelinek @ 2023-07-19 13:11 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches On Tue, Jul 18, 2023 at 01:25:45PM +0200, Richard Biener wrote: > > PR middle-end/61747 > * internal-fn.cc (expand_vec_cond_optab_fn): When the > value operands are equal to the original comparison operands > preserve that equality by re-using the comparison expansion. > * optabs.cc (emit_conditional_move): When the value operands > are equal to the comparison operands and would be forced to > a register by prepare_cmp_insn do so earlier, preserving the > equality. > > * g++.target/i386/pr61747.C: New testcase. > --- > gcc/internal-fn.cc | 17 ++++++++-- > gcc/optabs.cc | 32 ++++++++++++++++++- > gcc/testsuite/g++.target/i386/pr61747.C | 42 +++++++++++++++++++++++++ > 3 files changed, 88 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/g++.target/i386/pr61747.C > > diff --git a/gcc/internal-fn.cc b/gcc/internal-fn.cc > index e698f0bffc7..c83c3921792 100644 > --- a/gcc/internal-fn.cc > +++ b/gcc/internal-fn.cc > @@ -3019,8 +3019,21 @@ expand_vec_cond_optab_fn (internal_fn, gcall *stmt, convert_optab optab) > icode = convert_optab_handler (optab, mode, cmp_op_mode); > rtx comparison > = vector_compare_rtx (VOIDmode, tcode, op0a, op0b, unsignedp, icode, 4); > - rtx rtx_op1 = expand_normal (op1); > - rtx rtx_op2 = expand_normal (op2); > + /* vector_compare_rtx legitimizes operands, preserve equality when > + expanding op1/op2. */ > + rtx rtx_op1, rtx_op2; > + if (operand_equal_p (op1, op0a)) > + rtx_op1 = XEXP (comparison, 0); > + else if (operand_equal_p (op1, op0b)) > + rtx_op1 = XEXP (comparison, 1); > + else > + rtx_op1 = expand_normal (op1); > + if (operand_equal_p (op2, op0a)) > + rtx_op2 = XEXP (comparison, 0); > + else if (operand_equal_p (op2, op0b)) > + rtx_op2 = XEXP (comparison, 1); > + else > + rtx_op2 = expand_normal (op2); > > rtx target = expand_expr (lhs, NULL_RTX, VOIDmode, EXPAND_WRITE); > create_output_operand (&ops[0], target, mode); The above LGTM, it relies on vector_compare_rtx not swapping the arguments or performing some other comparison canonicalization, but at least right now that function doesn't seem to do that. > --- a/gcc/optabs.cc > +++ b/gcc/optabs.cc > @@ -5119,13 +5119,43 @@ emit_conditional_move (rtx target, struct rtx_comparison comp, > last = get_last_insn (); > do_pending_stack_adjust (); > machine_mode cmpmode = comp.mode; > + rtx orig_op0 = XEXP (comparison, 0); > + rtx orig_op1 = XEXP (comparison, 1); > + rtx op2p = op2; > + rtx op3p = op3; > + /* If we are optimizing, force expensive constants into a register > + but preserve an eventual equality with op2/op3. */ > + if (CONSTANT_P (orig_op0) && optimize > + && (rtx_cost (orig_op0, mode, COMPARE, 0, > + optimize_insn_for_speed_p ()) > + > COSTS_N_INSNS (1)) > + && can_create_pseudo_p ()) > + { > + XEXP (comparison, 0) = force_reg (cmpmode, orig_op0); > + if (rtx_equal_p (orig_op0, op2)) > + op2p = XEXP (comparison, 0); > + if (rtx_equal_p (orig_op0, op3)) > + op3p = XEXP (comparison, 0); > + } > + if (CONSTANT_P (orig_op1) && optimize > + && (rtx_cost (orig_op1, mode, COMPARE, 0, > + optimize_insn_for_speed_p ()) > + > COSTS_N_INSNS (1)) > + && can_create_pseudo_p ()) > + { > + XEXP (comparison, 1) = force_reg (cmpmode, orig_op1); > + if (rtx_equal_p (orig_op1, op2)) > + op2p = XEXP (comparison, 1); > + if (rtx_equal_p (orig_op1, op3)) > + op3p = XEXP (comparison, 1); > + } I'm worried here, because prepare_cmp_insn before doing almost identical forcing to reg does if (CONST_SCALAR_INT_P (y)) canonicalize_comparison (mode, &comparison, &y); which the above change will make not happen anymore (for the more expensive constants). If we have a match between at least one of the comparison operands and op2/op3, I think having equivalency there is perhaps more important than the canonicalization, but it would be nice not to break it even if there is no match. So, perhaps force_reg only if there is a match? force_reg (cmpmode, force_reg (cmpmode, x)) is equivalent to force_reg (cmpmode, x), so perhaps: { if (rtx_equal_p (orig_op0, op2)) op2p = XEXP (comparison, 0) = force_reg (cmpmode, orig_op0); if (rtx_equal_p (orig_op0, op3)) op3p = XEXP (comparison, 0) = force_reg (cmpmode, XEXP (comparison, 0)); } and similarly for the other body? Jakub ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] middle-end/61747 - conditional move expansion and constants 2023-07-19 13:11 ` Jakub Jelinek @ 2023-07-19 13:36 ` Richard Biener 2023-07-19 13:39 ` Jakub Jelinek 0 siblings, 1 reply; 4+ messages in thread From: Richard Biener @ 2023-07-19 13:36 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches On Wed, 19 Jul 2023, Jakub Jelinek wrote: > On Tue, Jul 18, 2023 at 01:25:45PM +0200, Richard Biener wrote: > > > > PR middle-end/61747 > > * internal-fn.cc (expand_vec_cond_optab_fn): When the > > value operands are equal to the original comparison operands > > preserve that equality by re-using the comparison expansion. > > * optabs.cc (emit_conditional_move): When the value operands > > are equal to the comparison operands and would be forced to > > a register by prepare_cmp_insn do so earlier, preserving the > > equality. > > > > * g++.target/i386/pr61747.C: New testcase. > > --- > > gcc/internal-fn.cc | 17 ++++++++-- > > gcc/optabs.cc | 32 ++++++++++++++++++- > > gcc/testsuite/g++.target/i386/pr61747.C | 42 +++++++++++++++++++++++++ > > 3 files changed, 88 insertions(+), 3 deletions(-) > > create mode 100644 gcc/testsuite/g++.target/i386/pr61747.C > > > > diff --git a/gcc/internal-fn.cc b/gcc/internal-fn.cc > > index e698f0bffc7..c83c3921792 100644 > > --- a/gcc/internal-fn.cc > > +++ b/gcc/internal-fn.cc > > @@ -3019,8 +3019,21 @@ expand_vec_cond_optab_fn (internal_fn, gcall *stmt, convert_optab optab) > > icode = convert_optab_handler (optab, mode, cmp_op_mode); > > rtx comparison > > = vector_compare_rtx (VOIDmode, tcode, op0a, op0b, unsignedp, icode, 4); > > - rtx rtx_op1 = expand_normal (op1); > > - rtx rtx_op2 = expand_normal (op2); > > + /* vector_compare_rtx legitimizes operands, preserve equality when > > + expanding op1/op2. */ > > + rtx rtx_op1, rtx_op2; > > + if (operand_equal_p (op1, op0a)) > > + rtx_op1 = XEXP (comparison, 0); > > + else if (operand_equal_p (op1, op0b)) > > + rtx_op1 = XEXP (comparison, 1); > > + else > > + rtx_op1 = expand_normal (op1); > > + if (operand_equal_p (op2, op0a)) > > + rtx_op2 = XEXP (comparison, 0); > > + else if (operand_equal_p (op2, op0b)) > > + rtx_op2 = XEXP (comparison, 1); > > + else > > + rtx_op2 = expand_normal (op2); > > > > rtx target = expand_expr (lhs, NULL_RTX, VOIDmode, EXPAND_WRITE); > > create_output_operand (&ops[0], target, mode); > > The above LGTM, it relies on vector_compare_rtx not swapping the arguments > or performing some other comparison canonicalization, but at least right now > that function doesn't seem to do that. > > > --- a/gcc/optabs.cc > > +++ b/gcc/optabs.cc > > @@ -5119,13 +5119,43 @@ emit_conditional_move (rtx target, struct rtx_comparison comp, > > last = get_last_insn (); > > do_pending_stack_adjust (); > > machine_mode cmpmode = comp.mode; > > + rtx orig_op0 = XEXP (comparison, 0); > > + rtx orig_op1 = XEXP (comparison, 1); > > + rtx op2p = op2; > > + rtx op3p = op3; > > + /* If we are optimizing, force expensive constants into a register > > + but preserve an eventual equality with op2/op3. */ > > + if (CONSTANT_P (orig_op0) && optimize > > + && (rtx_cost (orig_op0, mode, COMPARE, 0, > > + optimize_insn_for_speed_p ()) > > + > COSTS_N_INSNS (1)) > > + && can_create_pseudo_p ()) > > + { > > + XEXP (comparison, 0) = force_reg (cmpmode, orig_op0); > > + if (rtx_equal_p (orig_op0, op2)) > > + op2p = XEXP (comparison, 0); > > + if (rtx_equal_p (orig_op0, op3)) > > + op3p = XEXP (comparison, 0); > > + } > > + if (CONSTANT_P (orig_op1) && optimize > > + && (rtx_cost (orig_op1, mode, COMPARE, 0, > > + optimize_insn_for_speed_p ()) > > + > COSTS_N_INSNS (1)) > > + && can_create_pseudo_p ()) > > + { > > + XEXP (comparison, 1) = force_reg (cmpmode, orig_op1); > > + if (rtx_equal_p (orig_op1, op2)) > > + op2p = XEXP (comparison, 1); > > + if (rtx_equal_p (orig_op1, op3)) > > + op3p = XEXP (comparison, 1); > > + } > > I'm worried here, because prepare_cmp_insn before doing almost identical > forcing to reg does > if (CONST_SCALAR_INT_P (y)) > canonicalize_comparison (mode, &comparison, &y); > which the above change will make not happen anymore (for the more expensive > constants). Hmm, yeah - that could happen. > If we have a match between at least one of the comparison operands and > op2/op3, I think having equivalency there is perhaps more important than > the canonicalization, but it would be nice not to break it even if there > is no match. So, perhaps force_reg only if there is a match? > force_reg (cmpmode, force_reg (cmpmode, x)) is equivalent to > force_reg (cmpmode, x), so perhaps: > { > if (rtx_equal_p (orig_op0, op2)) > op2p = XEXP (comparison, 0) = force_reg (cmpmode, orig_op0); > if (rtx_equal_p (orig_op0, op3)) > op3p = XEXP (comparison, 0) > = force_reg (cmpmode, XEXP (comparison, 0)); > } > and similarly for the other body? I don't think we'll have op3 == op2 == orig_op0 because if op2 == op3 the /* If the two source operands are identical, that's just a move. */ if (rtx_equal_p (op2, op3)) { if (!target) target = gen_reg_rtx (mode); emit_move_insn (target, op3); return target; code should have triggered. So we should know we invoke force_reg only once for each comparison operand check? So I'm going to test the following ontop of the patch. Thanks, Richard. diff --git a/gcc/optabs.cc b/gcc/optabs.cc index a9ba3267666..2ac4b2698b2 100644 --- a/gcc/optabs.cc +++ b/gcc/optabs.cc @@ -5131,11 +5131,10 @@ emit_conditional_move (rtx target, struct rtx_comparison comp, > COSTS_N_INSNS (1)) && can_create_pseudo_p ()) { - XEXP (comparison, 0) = force_reg (cmpmode, orig_op0); if (rtx_equal_p (orig_op0, op2)) - op2p = XEXP (comparison, 0); + op2p = XEXP (comparison, 0) = force_reg (cmpmode, orig_op0); if (rtx_equal_p (orig_op0, op3)) - op3p = XEXP (comparison, 0); + op3p = XEXP (comparison, 0) = force_reg (cmpmode, orig_op0); } if (CONSTANT_P (orig_op1) && optimize && (rtx_cost (orig_op1, mode, COMPARE, 0, @@ -5143,11 +5142,10 @@ emit_conditional_move (rtx target, struct rtx_comparison comp, > COSTS_N_INSNS (1)) && can_create_pseudo_p ()) { - XEXP (comparison, 1) = force_reg (cmpmode, orig_op1); if (rtx_equal_p (orig_op1, op2)) - op2p = XEXP (comparison, 1); + op2p = XEXP (comparison, 1) = force_reg (cmpmode, orig_op1); if (rtx_equal_p (orig_op1, op3)) - op3p = XEXP (comparison, 1); + op3p = XEXP (comparison, 1) = force_reg (cmpmode, orig_op1); } prepare_cmp_insn (XEXP (comparison, 0), XEXP (comparison, 1), GET_CODE (comparison), NULL_RTX, unsignedp, ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] middle-end/61747 - conditional move expansion and constants 2023-07-19 13:36 ` Richard Biener @ 2023-07-19 13:39 ` Jakub Jelinek 0 siblings, 0 replies; 4+ messages in thread From: Jakub Jelinek @ 2023-07-19 13:39 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches On Wed, Jul 19, 2023 at 01:36:23PM +0000, Richard Biener wrote: > > If we have a match between at least one of the comparison operands and > > op2/op3, I think having equivalency there is perhaps more important than > > the canonicalization, but it would be nice not to break it even if there > > is no match. So, perhaps force_reg only if there is a match? > > force_reg (cmpmode, force_reg (cmpmode, x)) is equivalent to > > force_reg (cmpmode, x), so perhaps: > > { > > if (rtx_equal_p (orig_op0, op2)) > > op2p = XEXP (comparison, 0) = force_reg (cmpmode, orig_op0); > > if (rtx_equal_p (orig_op0, op3)) > > op3p = XEXP (comparison, 0) > > = force_reg (cmpmode, XEXP (comparison, 0)); > > } > > and similarly for the other body? > > I don't think we'll have op3 == op2 == orig_op0 because if > op2 == op3 the > > /* If the two source operands are identical, that's just a move. */ > > if (rtx_equal_p (op2, op3)) > { > if (!target) > target = gen_reg_rtx (mode); > > emit_move_insn (target, op3); > return target; > > code should have triggered. So we should know we invoke force_reg > only once for each comparison operand check? > > So I'm going to test the following ontop of the patch. Please use else if instead of the second if then. Ok with that change. > --- a/gcc/optabs.cc > +++ b/gcc/optabs.cc > @@ -5131,11 +5131,10 @@ emit_conditional_move (rtx target, struct > rtx_comparison comp, > > COSTS_N_INSNS (1)) > && can_create_pseudo_p ()) > { > - XEXP (comparison, 0) = force_reg (cmpmode, orig_op0); > if (rtx_equal_p (orig_op0, op2)) > - op2p = XEXP (comparison, 0); > + op2p = XEXP (comparison, 0) = force_reg (cmpmode, > orig_op0); > if (rtx_equal_p (orig_op0, op3)) > - op3p = XEXP (comparison, 0); > + op3p = XEXP (comparison, 0) = force_reg (cmpmode, > orig_op0); > } > if (CONSTANT_P (orig_op1) && optimize > && (rtx_cost (orig_op1, mode, COMPARE, 0, > @@ -5143,11 +5142,10 @@ emit_conditional_move (rtx target, struct > rtx_comparison comp, > > COSTS_N_INSNS (1)) > && can_create_pseudo_p ()) > { > - XEXP (comparison, 1) = force_reg (cmpmode, orig_op1); > if (rtx_equal_p (orig_op1, op2)) > - op2p = XEXP (comparison, 1); > + op2p = XEXP (comparison, 1) = force_reg (cmpmode, > orig_op1); > if (rtx_equal_p (orig_op1, op3)) > - op3p = XEXP (comparison, 1); > + op3p = XEXP (comparison, 1) = force_reg (cmpmode, > orig_op1); > } > prepare_cmp_insn (XEXP (comparison, 0), XEXP (comparison, 1), > GET_CODE (comparison), NULL_RTX, unsignedp, Jakub ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-07-19 13:39 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-07-18 11:25 [PATCH] middle-end/61747 - conditional move expansion and constants Richard Biener 2023-07-19 13:11 ` Jakub Jelinek 2023-07-19 13:36 ` Richard Biener 2023-07-19 13:39 ` Jakub Jelinek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).