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 <ma...@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
