On 5/19/23 02:49, Georg-Johann Lay wrote:
Subject:
Re: [patch,avr] Fix PR109650 wrong code
From:
Georg-Johann Lay <a...@gjlay.de>
Date:
5/19/23, 02:49

To:
gcc-patches@gcc.gnu.org
CC:
Jeff Law <jeffreya...@gmail.com>, Denis Chertykov <cherty...@gmail.com>, Senthil Kumar Selvaraj <senthilkumar.selva...@microchip.com>


...Ok, and now with the patch attached...

Here is a revised version of the patch.  The difference to the
previous one is that it adds some combine patterns for *cbranch
insns that were lost in the PR92729 transition.  The post-reload
part of the patterns were still there.  The new patterns are
slightly more general in that they also handle fixed-point modes.

Apart from that, the patch behaves the same:

Am 15.05.23 um 20:05 schrieb Georg-Johann Lay:
This patch fixes a wrong-code bug in the wake of PR92729, the transition
that turned the AVR backend from cc0 to CCmode.  In cc0, the insn that
uses cc0 like a conditional branch always follows the cc0 setter, which
is no more the case with CCmode where set and use of REG_CC might be in
different basic blocks.

This patch removes the machine-dependent reorg pass in avr_reorg entirely.

It is replaced by a new, AVR specific mini-pass that runs prior to
split2. Canonicalization of comparisons away from the "difficult"
codes GT[U] and LE[U] is now mostly performed by implementing
TARGET_CANONICALIZE_COMPARISON.

Moreover:

* Text peephole conditions get "dead_or_set_regno_p (*, REG_CC)" as
needed.

* RTL peephole conditions get "peep2_regno_dead_p (*, REG_CC)" as
needed.

* Conditional branches no more clobber REG_CC.

* insn output for compares looks ahead to determine the branch mode in
use. This needs also "dead_or_set_regno_p (*, REG_CC)".

* Add RTL peepholes for decrement-and-branch detection.

Finally, it fixes some of the many indentation glitches left over from
PR92729.

Ok?

I'd also backport this one because all of v12+ is affected by the wrong code.

Johann

--

gcc/
     PR target/109650
     PR target/92729

     * config/avr/avr-passes.def (avr_pass_ifelse): Insert new pass.
     * config/avr/avr.cc (avr_pass_ifelse): New RTL pass.
     (avr_pass_data_ifelse): New pass_data for it.
     (make_avr_pass_ifelse, avr_redundant_compare, avr_cbranch_cost)
     (avr_canonicalize_comparison, avr_out_plus_set_ZN)
     (avr_out_cmp_ext): New functions.
     (compare_condtition): Make sure REG_CC dies in the branch insn.
     (avr_rtx_costs_1): Add computation of cbranch costs.
     (avr_adjust_insn_length) [ADJUST_LEN_ADD_SET_ZN, ADJUST_LEN_CMP_ZEXT]:
     [ADJUST_LEN_CMP_SEXT]Handle them.
     (TARGET_CANONICALIZE_COMPARISON): New define.
     (avr_simplify_comparison_p, compare_diff_p, avr_compare_pattern)
     (avr_reorg_remove_redundant_compare, avr_reorg): Remove functions.
     (TARGET_MACHINE_DEPENDENT_REORG): Remove define.

     * avr-protos.h (avr_simplify_comparison_p): Remove proto.
     (make_avr_pass_ifelse, avr_out_plus_set_ZN, cc_reg_rtx)
     (avr_out_cmp_zext): New Protos

     * config/avr/avr.md (branch, difficult_branch): Don't split insns.
     (*cbranchhi.zero-extend.0", *cbranchhi.zero-extend.1")
     (*swapped_tst<mode>, *add.for.eqne.<mode>): New insns.
     (*cbranch<mode>4): Rename to cbranch<mode>4_insn.
     (define_peephole): Add dead_or_set_regno_p(insn,REG_CC) as needed.
     (define_deephole2): Add peep2_regno_dead_p(*,REG_CC) as needed.
     Add new RTL peepholes for decrement-and-branch and *swapped_tst<mode>.
     Rework signtest-and-branch peepholes for *sbrx_branch<mode>.
     (adjust_len) [add_set_ZN, cmp_zext]: New.
     (QIPSI): New mode iterator.
     (ALLs1, ALLs2, ALLs4, ALLs234): New mode iterators.
     (gelt): New code iterator.
     (gelt_eqne): New code attribute.
     (rvbranch, *rvbranch, difficult_rvbranch, *difficult_rvbranch)
     (branch_unspec, *negated_tst<mode>, *reversed_tst<mode>)
     (*cmpqi_sign_extend): Remove insns.
     (define_c_enum "unspec") [UNSPEC_IDENTITY]: Remove.

     * config/avr/avr-dimode.md (cbranch<mode>4): Canonicalize comparisons.
     * config/avr/predicates.md (scratch_or_d_register_operand): New.
     * config/avr/contraints.md (Yxx): New constraint.

gcc/testsuite/
     PR target/109650
     * config/avr/torture/pr109650-1.c: New test.
     * config/avr/torture/pr109650-2.c: New test.
So I did a cursory review and didn't see anything obviously wrong. Given we haven't heard from Denis in a while I'll go ahead and ACK.

More importantly how do we want to handle things going forward? I don't think my involvement in avr specific changes would bring any significant value and I don't expect to hear from Denis.

I could propose you as the maintainer of the avr port to the steering committee, but I wouldn't do that without knowing its a task you want to take on.


Jeff

Reply via email to