Re: Ping: [PATCHv3] aarch64/expr: Use ccmp when the outer expression is used twice [PR100942]
On Mon, Jan 22, 2024 at 2:24 PM Richard Sandiford
wrote:
>
> Ping for the expr/cfgexpand bits
>
> Richard Sandiford writes:
> > Andrew Pinski writes:
> >> Ccmp is not used if the result of the and/ior is used by both
> >> a GIMPLE_COND and a GIMPLE_ASSIGN. This improves the code generation
> >> here by using ccmp in this case.
> >> Two changes is required, first we need to allow the outer statement's
> >> result be used more than once.
> >> The second change is that during the expansion of the gimple, we need
> >> to try using ccmp. This is needed because we don't use expand the ssa
> >> name of the lhs but rather expand directly from the gimple.
> >>
> >> A small note on the ccmp_4.c testcase, we should be able to get slightly
> >> better than with this patch but it is one extra instruction compared to
> >> before.
> >>
> >> Diff from v1:
> >> * v2: Split out expand_gimple_assign_ssa so the we only need to handle
> >> promotion once. Add ccmp_5.c testcase which was suggested. Change comment
> >> on ccmp_candidate_p.
> >
> > I meant more that we should split out the gassign handling in
> > expand_expr_real_1, since we're effectively making cfgexpand follow
> > it more closely. What do you think about the attached version?
> > Tested on aarch64-linux-gnu and x86_64-linux-gnu.
> >
> > OK for the expr/cfgexpand bits?
OK.
Richard.
> > Thanks,
> > Richard
> >
> >
> >
> > Ccmp is not used if the result of the and/ior is used by both
> > a GIMPLE_COND and a GIMPLE_ASSIGN. This improves the code generation
> > here by using ccmp in this case.
> > Two changes is required, first we need to allow the outer statement's
> > result be used more than once.
> > The second change is that during the expansion of the gimple, we need
> > to try using ccmp. This is needed because we don't use expand the ssa
> > name of the lhs but rather expand directly from the gimple.
> >
> > A small note on the ccmp_4.c testcase, we should be able to get slightly
> > better than with this patch but it is one extra instruction compared to
> > before.
> >
> > PR target/100942
> >
> > gcc/ChangeLog:
> >
> > * ccmp.cc (ccmp_candidate_p): Add outer argument.
> > Allow if the outer is true and the lhs is used more
> > than once.
> > (expand_ccmp_expr): Update call to ccmp_candidate_p.
> > * expr.h (expand_expr_real_gassign): Declare.
> > * expr.cc (expand_expr_real_gassign): New function, split out from...
> > (expand_expr_real_1): ...here.
> > * cfgexpand.cc (expand_gimple_stmt_1): Use expand_expr_real_gassign.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.target/aarch64/ccmp_3.c: New test.
> > * gcc.target/aarch64/ccmp_4.c: New test.
> > * gcc.target/aarch64/ccmp_5.c: New test.
> >
> > Signed-off-by: Andrew Pinski
> > Co-authored-by: Richard Sandiford
> > ---
> > gcc/ccmp.cc | 12 +--
> > gcc/cfgexpand.cc | 31 ++-
> > gcc/expr.cc | 103 --
> > gcc/expr.h| 3 +
> > gcc/testsuite/gcc.target/aarch64/ccmp_3.c | 20 +
> > gcc/testsuite/gcc.target/aarch64/ccmp_4.c | 35
> > gcc/testsuite/gcc.target/aarch64/ccmp_5.c | 20 +
> > 7 files changed, 149 insertions(+), 75 deletions(-)
> > create mode 100644 gcc/testsuite/gcc.target/aarch64/ccmp_3.c
> > create mode 100644 gcc/testsuite/gcc.target/aarch64/ccmp_4.c
> > create mode 100644 gcc/testsuite/gcc.target/aarch64/ccmp_5.c
> >
> > diff --git a/gcc/ccmp.cc b/gcc/ccmp.cc
> > index 09d6b5595a4..7cb525addf4 100644
> > --- a/gcc/ccmp.cc
> > +++ b/gcc/ccmp.cc
> > @@ -90,9 +90,10 @@ ccmp_tree_comparison_p (tree t, basic_block bb)
> > If all checks OK in expand_ccmp_expr, it emits insns in prep_seq, then
> > insns in gen_seq. */
> >
> > -/* Check whether G is a potential conditional compare candidate. */
> > +/* Check whether G is a potential conditional compare candidate; OUTER is
> > true if
> > + G is the outer most AND/IOR. */
> > static bool
> > -ccmp_candidate_p (gimple *g)
> > +ccmp_candidate_p (gimple *g, bool outer = false)
> > {
> >tree lhs, op0, op1;
> >gimple *gs0, *gs1;
> > @@ -109,8 +110,9 @@ ccmp_candidate_p (gimple *g)
> >lhs = gimple_assign_lhs (g);
> >op0 = gimple_assign_rhs1 (g);
> >op1 = gimple_assign_rhs2 (g);
> > - if ((TREE_CODE (op0) != SSA_NAME) || (TREE_CODE (op1) != SSA_NAME)
> > - || !has_single_use (lhs))
> > + if ((TREE_CODE (op0) != SSA_NAME) || (TREE_CODE (op1) != SSA_NAME))
> > +return false;
> > + if (!outer && !has_single_use (lhs))
> > return false;
> >
> >bb = gimple_bb (g);
> > @@ -284,7 +286,7 @@ expand_ccmp_expr (gimple *g, machine_mode mode)
> >rtx_insn *last;
> >rtx tmp;
> >
> > - if (!ccmp_candidate_p (g))
> > + if (!ccmp_candidate_p (g, true))
> > return NULL_RTX;
> >
> >last = get_last_insn ();
> > diff --git a/gcc/cfgexpand.cc
Ping: [PATCHv3] aarch64/expr: Use ccmp when the outer expression is used twice [PR100942]
Ping for the expr/cfgexpand bits
Richard Sandiford writes:
> Andrew Pinski writes:
>> Ccmp is not used if the result of the and/ior is used by both
>> a GIMPLE_COND and a GIMPLE_ASSIGN. This improves the code generation
>> here by using ccmp in this case.
>> Two changes is required, first we need to allow the outer statement's
>> result be used more than once.
>> The second change is that during the expansion of the gimple, we need
>> to try using ccmp. This is needed because we don't use expand the ssa
>> name of the lhs but rather expand directly from the gimple.
>>
>> A small note on the ccmp_4.c testcase, we should be able to get slightly
>> better than with this patch but it is one extra instruction compared to
>> before.
>>
>> Diff from v1:
>> * v2: Split out expand_gimple_assign_ssa so the we only need to handle
>> promotion once. Add ccmp_5.c testcase which was suggested. Change comment
>> on ccmp_candidate_p.
>
> I meant more that we should split out the gassign handling in
> expand_expr_real_1, since we're effectively making cfgexpand follow
> it more closely. What do you think about the attached version?
> Tested on aarch64-linux-gnu and x86_64-linux-gnu.
>
> OK for the expr/cfgexpand bits?
>
> Thanks,
> Richard
>
>
>
> Ccmp is not used if the result of the and/ior is used by both
> a GIMPLE_COND and a GIMPLE_ASSIGN. This improves the code generation
> here by using ccmp in this case.
> Two changes is required, first we need to allow the outer statement's
> result be used more than once.
> The second change is that during the expansion of the gimple, we need
> to try using ccmp. This is needed because we don't use expand the ssa
> name of the lhs but rather expand directly from the gimple.
>
> A small note on the ccmp_4.c testcase, we should be able to get slightly
> better than with this patch but it is one extra instruction compared to
> before.
>
> PR target/100942
>
> gcc/ChangeLog:
>
> * ccmp.cc (ccmp_candidate_p): Add outer argument.
> Allow if the outer is true and the lhs is used more
> than once.
> (expand_ccmp_expr): Update call to ccmp_candidate_p.
> * expr.h (expand_expr_real_gassign): Declare.
> * expr.cc (expand_expr_real_gassign): New function, split out from...
> (expand_expr_real_1): ...here.
> * cfgexpand.cc (expand_gimple_stmt_1): Use expand_expr_real_gassign.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/aarch64/ccmp_3.c: New test.
> * gcc.target/aarch64/ccmp_4.c: New test.
> * gcc.target/aarch64/ccmp_5.c: New test.
>
> Signed-off-by: Andrew Pinski
> Co-authored-by: Richard Sandiford
> ---
> gcc/ccmp.cc | 12 +--
> gcc/cfgexpand.cc | 31 ++-
> gcc/expr.cc | 103 --
> gcc/expr.h| 3 +
> gcc/testsuite/gcc.target/aarch64/ccmp_3.c | 20 +
> gcc/testsuite/gcc.target/aarch64/ccmp_4.c | 35
> gcc/testsuite/gcc.target/aarch64/ccmp_5.c | 20 +
> 7 files changed, 149 insertions(+), 75 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/aarch64/ccmp_3.c
> create mode 100644 gcc/testsuite/gcc.target/aarch64/ccmp_4.c
> create mode 100644 gcc/testsuite/gcc.target/aarch64/ccmp_5.c
>
> diff --git a/gcc/ccmp.cc b/gcc/ccmp.cc
> index 09d6b5595a4..7cb525addf4 100644
> --- a/gcc/ccmp.cc
> +++ b/gcc/ccmp.cc
> @@ -90,9 +90,10 @@ ccmp_tree_comparison_p (tree t, basic_block bb)
> If all checks OK in expand_ccmp_expr, it emits insns in prep_seq, then
> insns in gen_seq. */
>
> -/* Check whether G is a potential conditional compare candidate. */
> +/* Check whether G is a potential conditional compare candidate; OUTER is
> true if
> + G is the outer most AND/IOR. */
> static bool
> -ccmp_candidate_p (gimple *g)
> +ccmp_candidate_p (gimple *g, bool outer = false)
> {
>tree lhs, op0, op1;
>gimple *gs0, *gs1;
> @@ -109,8 +110,9 @@ ccmp_candidate_p (gimple *g)
>lhs = gimple_assign_lhs (g);
>op0 = gimple_assign_rhs1 (g);
>op1 = gimple_assign_rhs2 (g);
> - if ((TREE_CODE (op0) != SSA_NAME) || (TREE_CODE (op1) != SSA_NAME)
> - || !has_single_use (lhs))
> + if ((TREE_CODE (op0) != SSA_NAME) || (TREE_CODE (op1) != SSA_NAME))
> +return false;
> + if (!outer && !has_single_use (lhs))
> return false;
>
>bb = gimple_bb (g);
> @@ -284,7 +286,7 @@ expand_ccmp_expr (gimple *g, machine_mode mode)
>rtx_insn *last;
>rtx tmp;
>
> - if (!ccmp_candidate_p (g))
> + if (!ccmp_candidate_p (g, true))
> return NULL_RTX;
>
>last = get_last_insn ();
> diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc
> index 1db22f0a1a3..381ed2c82d7 100644
> --- a/gcc/cfgexpand.cc
> +++ b/gcc/cfgexpand.cc
> @@ -3971,37 +3971,18 @@ expand_gimple_stmt_1 (gimple *stmt)
> {
> rtx target, temp;
> bool nontemporal = gimple_assign_nontemporal_move_p (assign_stmt);
> - struct
