Hi Michiel, Thank you for the detailed review and helpful advice.
> Note that if your microarchtecture always matches add & addi encoding, > these should also be considered: > `mv rd, rs` is encoded as `addi rd, rs, 0` > `c.mv rs, rs` as `c.add rd, x0, rs` > `li c` as `addi rd, x0, c` Good point. The C950 fusion unit operates at the decode stage on the actual encoding, so `mv`/`li` (which are `addi` encodings) and `c.mv` (a `c.add` encoding) are indeed fusible when followed or preceded by a matching second instruction. At the RTL level these appear as plain `(set reg reg)` or `(set reg const_int)` and never go through `riscv_set_is_add_addi_p`, so they are currently missed. I will add matching for these RTL forms in the relevant checkers (ADD_ANDI, ANDI_ADD, etc.) in v2. > >+ rtx dummy0, dummy1; > >+ if (!src0) src0 = &dummy0; > >+ if (!src1) src1 = &dummy1; > > I'm not sure this is valid. In any case, probably a batter idea to > check whether src0/1 is `NULL` explicitly. You are right, the dummy approach is ugly even if technically valid. I will switch to explicit NULL checks before each `*src0`/`*src1` assignment in v2. > We had trouble getting this to be generated properly with the matching > destination register because of this pattern: > > (define_insn_and_split "*<any_extract:optab><GPR:mode>3" > ... > (clobber (match_scratch:GPR 4 "=&r"))] > ... > "&& reload_completed" > [(set (match_dup 4) > (ashift:GPR (match_dup 1) (match_dup 2))) > (set (match_dup 0) > (<extract_shift>:GPR (match_dup 4) (match_dup 3)))] > > This might be less of an issue since GCC 16 now that there is logic > that tries to use the same destination register in RA/regrename for > fused pairs. Perhaps the `reload_completed` needs to be ditched here, > might give better results. Thanks for pointing this out. I looked into this and the scratch is actually unnecessary: slli reads its source before writing the destination, so operand 0 can safely serve as the intermediate — no early-clobber needed. Removing the scratch eliminates the register mismatch entirely, independent of regrename behavior. I have submitted a separate patch for this: https://patchwork.sourceware.org/project/gcc/patch/[email protected]/ The `reload_completed` split condition is kept because removing it risks combiner loops (the combiner would merge the two shifts back into zero_extract, which would immediately split again). --- I will hold off on posting v2 of the fusion series for now and wait for further review comments from other maintainers / experts. Once I have collected all the feedback I will address everything together in the next revision. If you have any further suggestions or concerns, they are very welcome. Thanks again for the thorough and insightful review. Best regards, Jin Ma
