On Fri, Aug 23, 2024 at 2:16 PM Georg-Johann Lay <[email protected]> wrote:
>
> This patch overhauls the avr-ifelse mini-pass that optimizes
> two cbranch insns to one comparison and two branches.
>
> More optimization opportunities are realized, and the code
> has been refactored.
>
> No new regressions. Ok for trunk?
>
> There is currently no avr maintainer, so some global reviewer
> might please have a look at this.
I see Denis still listed? Possibly Jeff can have a look though.
> And one question I have: avr_optimize_2ifelse() is rewiring
> basic blocks using redirect_edge_and_branch(). Does this
> require extra pass flags or actions? Currently the RTL_PASS
> data reads:
It probably depends where the pass is inserted, but if it didn't
blow up spectacularly in your testing it should be fine as-is.
> static const pass_data avr_pass_data_ifelse =
> {
> RTL_PASS, // type
> "", // name (will be patched)
> OPTGROUP_NONE, // optinfo_flags
> TV_DF_SCAN, // tv_id
> 0, // properties_required
> 0, // properties_provided
> 0, // properties_destroyed
> 0, // todo_flags_start
> TODO_df_finish | TODO_df_verify // todo_flags_finish
> };
>
>
> Johann
>
> p.s. The additional notes on compare-elim / PR115830 can be found
> here (pending review):
>
> https://gcc.gnu.org/pipermail/gcc-patches/2024-August/660743.html
>
> --
>
> AVR: Overhaul the avr-ifelse RTL optimization pass.
>
> Mini-pass avr-ifelse realizes optimizations that replace two cbranch
> insns with one comparison and two branches. This patch adds the
> following improvements:
>
> - The right operand of the comparisons may also be REGs.
> Formerly only CONST_INT was handled.
>
> - The code of the first comparison in no more restricted
> to (effectively) EQ.
>
> - When the second cbranch is located in the fallthrough path
> of the first cbranch, then difficult (expensive) comparisons
> can always be avoided. This may require to swap the branch
> targets. (When the second cbranch if located after the target
> label of the first one, then getting rid of difficult branches
> would require to reorder blocks.)
>
> - The code has been cleaned up: avr_rest_of_handle_ifelse() now
> just scans the insn stream for optimization candidates. The code
> that actually performs the transformation has been outsourced to
> the new function avr_optimize_2ifelse().
>
> - The code to find a better representation for reg-const_int comparisons
> has been split into two parts: First try to find codes such that the
> right-hand sides of the comparisons are the same (avr_2comparisons_rhs).
> When this succeeds then one comparison can serve two branches, and
> avr_redundant_compare() tries to get rid of difficult branches that
> may have been introduced by avr_2comparisons_rhs(). This is always
> possible when the second cbranch is located in the fallthrough path
> of the first one, or when the first code is EQ.
>
> Some final notes on why we don't use compare-elim: 1) The two cbranch
> insns may come with different scratch operands depending on the chosen
> constraint alternatives. There are cases where the outgoing comparison
> requires a scratch but only one incoming cbranch has one. 2) Avoiding
> difficult branches can be achieved by rewiring basic blocks.
> compare-elim doesn't do that; it doesn't even know the costs of the
> branch codes. 3) avr_2comparisons_rhs() may de-canonicalize a
> comparison to achieve its goal. compare-elim doesn't know how to do
> that. 4) There are more reasons, see for example the commit
> message and discussion for PR115830.
>
> gcc/
> * config/avr/avr.cc (cfganal.h): Include it.
> (avr_2comparisons_rhs, avr_redundant_compare_regs)
> (avr_strict_signed_p, avr_strict_unsigned_p): New static functions.
> (avr_redundant_compare): Overhaul: Allow more cases.
> (avr_optimize_2ifelse): New static function, outsourced from...
> (avr_rest_of_handle_ifelse): ...this method.
> gcc/testsuite/
> * gcc.target/avr/torture/ifelse-c.h: New file.
> * gcc.target/avr/torture/ifelse-d.h: New file.
> * gcc.target/avr/torture/ifelse-q.h: New file.
> * gcc.target/avr/torture/ifelse-r.h: New file.
> * gcc.target/avr/torture/ifelse-c-i8.c: New test.
> * gcc.target/avr/torture/ifelse-d-i8.c: New test.
> * gcc.target/avr/torture/ifelse-q-i8.c: New test.
> * gcc.target/avr/torture/ifelse-r-i8.c: New test.
> * gcc.target/avr/torture/ifelse-c-i16.c: New test.
> * gcc.target/avr/torture/ifelse-d-i16.c: New test.
> * gcc.target/avr/torture/ifelse-q-i16.c: New test.
> * gcc.target/avr/torture/ifelse-r-i16.c: New test.
> * gcc.target/avr/torture/ifelse-c-u16.c: New test.
> * gcc.target/avr/torture/ifelse-d-u16.c: New test.
> * gcc.target/avr/torture/ifelse-q-u16.c: New test.
> * gcc.target/avr/torture/ifelse-r-u16.c: New test.
