| Message ID | 20231127043333.1955900-1-quic_apinski@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | aarch64: Improve cost of `a ? {-,}1 : b` | expand |
Andrew Pinski <quic_apinski@quicinc.com> writes: > While looking into PR 112454, I found the cost for > `(if_then_else (cmp) (const_int 1) (reg))` was being recorded as 8 > (or `COSTS_N_INSNS (2)`) but it should have been 4 (or `COSTS_N_INSNS (1)`). > This improves the cost by not adding the cost of `(const_int 1)` to > the total cost. > > It does not does not fix PR 112454 as that requires other changes to forwprop > the `(const_int 1)` earlier than combine. > > Bootstrapped and tested on aarch64-linux-gnu with no regressions. > > gcc/ChangeLog: > > * config/aarch64/aarch64.cc (aarch64_if_then_else_costs): > Don't add the cost of `1` or `-1`. > > Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com> > --- > gcc/config/aarch64/aarch64.cc | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > index f6f6f94bf43..63241c5aaa5 100644 > --- a/gcc/config/aarch64/aarch64.cc > +++ b/gcc/config/aarch64/aarch64.cc > @@ -11642,9 +11642,16 @@ aarch64_if_then_else_costs (rtx op0, rtx op1, rtx op2, int *cost, bool speed) > /* CSINV/NEG with zero extend + const 0 (*csinv3_uxtw_insn3). */ > op1 = XEXP (inner, 0); > } > - > - *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); > - *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 2, speed); > + if (op2 == constm1_rtx || op2 == const1_rtx) > + *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); > + else if (op1 == constm1_rtx || op1 == const1_rtx) > + *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 2, speed); It looks like this is really an extra option on top of the previous if-else chain, since it only applies when OP1 and OP2 are still the operands of the if_then_else. So how about: else if (op1 == constm1_rtx || op1 == const1_rtx) { /* Use CSINV. */ *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 2, speed); return true; } else if (op2 == constm1_rtx || op2 == const1_rtx) { /* Use CSINV. */ *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); return true; } leaving the code to fall through to: *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 2, speed); return true; as it does currently. OK in that form if you agree. Let me know if you don't. But in that case: > + else > + { > + *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); > + *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 1, speed); should be 2, speed > + } > + Thanks, Richard
Richard Sandiford <richard.sandiford@arm.com> writes: > Andrew Pinski <quic_apinski@quicinc.com> writes: >> While looking into PR 112454, I found the cost for >> `(if_then_else (cmp) (const_int 1) (reg))` was being recorded as 8 >> (or `COSTS_N_INSNS (2)`) but it should have been 4 (or `COSTS_N_INSNS (1)`). >> This improves the cost by not adding the cost of `(const_int 1)` to >> the total cost. >> >> It does not does not fix PR 112454 as that requires other changes to forwprop >> the `(const_int 1)` earlier than combine. >> >> Bootstrapped and tested on aarch64-linux-gnu with no regressions. >> >> gcc/ChangeLog: >> >> * config/aarch64/aarch64.cc (aarch64_if_then_else_costs): >> Don't add the cost of `1` or `-1`. >> >> Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com> >> --- >> gcc/config/aarch64/aarch64.cc | 13 ++++++++++--- >> 1 file changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc >> index f6f6f94bf43..63241c5aaa5 100644 >> --- a/gcc/config/aarch64/aarch64.cc >> +++ b/gcc/config/aarch64/aarch64.cc >> @@ -11642,9 +11642,16 @@ aarch64_if_then_else_costs (rtx op0, rtx op1, rtx op2, int *cost, bool speed) >> /* CSINV/NEG with zero extend + const 0 (*csinv3_uxtw_insn3). */ >> op1 = XEXP (inner, 0); >> } >> - >> - *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); >> - *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 2, speed); >> + if (op2 == constm1_rtx || op2 == const1_rtx) >> + *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); >> + else if (op1 == constm1_rtx || op1 == const1_rtx) >> + *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 2, speed); > > It looks like this is really an extra option on top of the previous > if-else chain, since it only applies when OP1 and OP2 are still the > operands of the if_then_else. So how about: > > else if (op1 == constm1_rtx || op1 == const1_rtx) > { > /* Use CSINV. */ eh, of course I meant CSINV or CSINC... > *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 2, speed); > return true; > } > else if (op2 == constm1_rtx || op2 == const1_rtx) > { > /* Use CSINV. */ > *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); > return true; > } > > leaving the code to fall through to: > > *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); > *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 2, speed); > return true; > > as it does currently. OK in that form if you agree. > > Let me know if you don't. But in that case: > >> + else >> + { >> + *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); >> + *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 1, speed); > > should be 2, speed > >> + } >> + > > Thanks, > Richard
> -----Original Message----- > From: Richard Sandiford <richard.sandiford@arm.com> > Sent: Monday, November 27, 2023 7:35 AM > To: Andrew Pinski (QUIC) <quic_apinski@quicinc.com> > Cc: gcc-patches@gcc.gnu.org > Subject: Re: [PATCH] aarch64: Improve cost of `a ? {-,}1 : b` > > Andrew Pinski <quic_apinski@quicinc.com> writes: > > While looking into PR 112454, I found the cost for `(if_then_else > > (cmp) (const_int 1) (reg))` was being recorded as 8 (or `COSTS_N_INSNS > > (2)`) but it should have been 4 (or `COSTS_N_INSNS (1)`). > > This improves the cost by not adding the cost of `(const_int 1)` to > > the total cost. > > > > It does not does not fix PR 112454 as that requires other changes to > > forwprop the `(const_int 1)` earlier than combine. > > > > Bootstrapped and tested on aarch64-linux-gnu with no regressions. > > > > gcc/ChangeLog: > > > > * config/aarch64/aarch64.cc (aarch64_if_then_else_costs): > > Don't add the cost of `1` or `-1`. > > > > Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com> > > --- > > gcc/config/aarch64/aarch64.cc | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/gcc/config/aarch64/aarch64.cc > > b/gcc/config/aarch64/aarch64.cc index f6f6f94bf43..63241c5aaa5 100644 > > --- a/gcc/config/aarch64/aarch64.cc > > +++ b/gcc/config/aarch64/aarch64.cc > > @@ -11642,9 +11642,16 @@ aarch64_if_then_else_costs (rtx op0, rtx op1, > rtx op2, int *cost, bool speed) > > /* CSINV/NEG with zero extend + const 0 (*csinv3_uxtw_insn3). */ > > op1 = XEXP (inner, 0); > > } > > - > > - *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); > > - *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 2, speed); > > + if (op2 == constm1_rtx || op2 == const1_rtx) > > + *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); > > + else if (op1 == constm1_rtx || op1 == const1_rtx) > > + *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 2, speed); > > It looks like this is really an extra option on top of the previous if-else chain, > since it only applies when OP1 and OP2 are still the operands of the > if_then_else. So how about: > > else if (op1 == constm1_rtx || op1 == const1_rtx) > { > /* Use CSINV. */ > *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 2, speed); > return true; > } > else if (op2 == constm1_rtx || op2 == const1_rtx) > { > /* Use CSINV. */ > *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); > return true; > } > > leaving the code to fall through to: > > *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); > *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 2, speed); > return true; > > as it does currently. OK in that form if you agree. Yes I think this is the correct way of implementing this, Let me test it and get back to you. Thanks, Andrew > > Let me know if you don't. But in that case: > > > + else > > + { > > + *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); > > + *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 1, speed); > > should be 2, speed > > > + } > > + > > Thanks, > Richard
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc index f6f6f94bf43..63241c5aaa5 100644 --- a/gcc/config/aarch64/aarch64.cc +++ b/gcc/config/aarch64/aarch64.cc @@ -11642,9 +11642,16 @@ aarch64_if_then_else_costs (rtx op0, rtx op1, rtx op2, int *cost, bool speed) /* CSINV/NEG with zero extend + const 0 (*csinv3_uxtw_insn3). */ op1 = XEXP (inner, 0); } - - *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); - *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 2, speed); + if (op2 == constm1_rtx || op2 == const1_rtx) + *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); + else if (op1 == constm1_rtx || op1 == const1_rtx) + *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 2, speed); + else + { + *cost += rtx_cost (op1, VOIDmode, IF_THEN_ELSE, 1, speed); + *cost += rtx_cost (op2, VOIDmode, IF_THEN_ELSE, 1, speed); + } + return true; }
While looking into PR 112454, I found the cost for `(if_then_else (cmp) (const_int 1) (reg))` was being recorded as 8 (or `COSTS_N_INSNS (2)`) but it should have been 4 (or `COSTS_N_INSNS (1)`). This improves the cost by not adding the cost of `(const_int 1)` to the total cost. It does not does not fix PR 112454 as that requires other changes to forwprop the `(const_int 1)` earlier than combine. Bootstrapped and tested on aarch64-linux-gnu with no regressions. gcc/ChangeLog: * config/aarch64/aarch64.cc (aarch64_if_then_else_costs): Don't add the cost of `1` or `-1`. Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com> --- gcc/config/aarch64/aarch64.cc | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)