Re: [PATCH 00/44] RISC-V: Various if-conversion fixes and improvements

[Kito Cheng <kito.cheng@gmail.com>]

quick response for this patch set, it's a really huge number of
patches, so I'll review it individually, and feel free to commit
individual one once got LGTM for each single patch :P


On Sun, Nov 19, 2023 at 1:35 PM Maciej W. Rozycki <macro@embecosm.com> wrote:
>
> Hi,
>
>  This patch series has come out from a simple change to add generic
> conditional-move and conditional-add expansions for a yet-out-of-tree
> target, which has relatively expensive branches and no conditional
> operations beyond the base architecture conditional-set instructions.  At
> one point I have concluded it may make sense to release this code to the
> general public, especially as some of the conditional execution sequences
> will trigger for targets we already have support for.  Naturally as a part
> of a proper upstream submission I chose to add suitable test cases.
>
>  Now these test cases triggered a lot of issues in our existing code and
> as I fixed them what was supposed to be a couple of patches has turned
> into this humongous patch series, including a branch costing model rework.
> Oh well.
>
>  Please see individual change descriptions for the details.  The overall
> patch series structure is as follows:
>
> - 01-02 add test cases covering the existing state that won't change
>   throughout the patch series,
>
> - 03-08 make small preparatory clean-ups that do not change semantics,
>
> - 09-13 implement a branch cost model rework and add the associated test
>   cases,
>
> - 14-24 make various improvements for integer conditional operations and
>   add the associated test cases,
>
> - 25-28 add generic `movMODEcc' support and the associated test cases,
>
> - 29-31 add generic `addMODEcc' support and the associated test cases,
>
> - 32-44 make various improvements for floating-point conditional
>   operations and add the associated test cases.
>
> There is potential here for middle end improvement, in particular branch
> costing is already documented in if-cvt.cc to be intended to consistently
> use BRANCH_COST, and then the generic conditional-move and conditional-add
> sequences could I suppose be emitted there in a target-agnostic way rather
> than being supplied by the backend.  This I suppose could be investigated
> in the future if the RISC-V approach turned out potentially useful for
> other targets.
>
>  This has been so far verified as follows, using SiFive HiFive Unmatched
> hardware and the `riscv64-linux-gnu' target:
>
> - New target test cases have been run with `-mtune=sifive-5-series',
>   `-mtune=sifive-5-series/-march=rv32gc/-mabi=ilp32d' and
>   `-mtune=sifive-5-series/-mmovcc/-mbranch-cost=8' DejaGNU board options.
>
> - The C language test suite has been run at significant points in the
>   patch series with `-mtune=sifive-5-series' and (past 26/44) also with
>   `-mtune=sifive-5-series/-mmovcc/-mbranch-cost=8', and selectively with
>   `-mtune=sifive-7-series' and
>   `-mtune=sifive-7-series/-mmovcc/-mbranch-cost=8' DejaGNU board options.
>
> Since this is huge and every test iteration takes a couple of hours I will
> continue running testing and may investigate running QEMU testing for the
> features the Unmatched does not support such as Zicond.  I don't expect
> real issues however.
>
>  There are a bunch of issues triggered with `-mmovcc/-mbranch-cost=8' or
> with lone `-mbranch-cost=8' even and the vector test cases, which are
> either due to match patterns expecting an assembly label that has been
> reordered or are similar to PR target/112092 and which are not a problem
> with this patch series, but rather one with the vector testsuite or code.
>
>  Any questions, comments, or concerns?  Otherwise OK to apply?
>
>   Maciej