Re: [PATCH 09/44] RISC-V: Rework branch costing model for if-conversion

["Maciej W. Rozycki" <macro@embecosm.com>]

On Sun, 19 Nov 2023, Jeff Law wrote:

> As I suspect you know a big part of the problem here is that BRANCH_COST and
> rtx_cost don't have any common scale and thus trying to compare BRANCH_COST to
> RTX_COST doesn't have well defined meaning.

 We do have preexisting places using COSTS_N_INSNS (BRANCH_COST ()) 
though, as documented in ifcvt.cc:

     ??? Actually, instead of the branch instruction costs we might want
     to use COSTS_N_INSNS (BRANCH_COST ()) as in other places.  */

so it seems the right direction, and given that we expose this measure to 
the user (and at the very least GCC developers implementing new tuning 
microarchitectures) I think it's the only sane way to do branch costing: 
define the measure in terms of how many ordinary ALU instructions a branch 
is statistically equivalent to.

 We may have to consider whether we want/need higher resolution here, 
effectively branch costs including a fractional part.

> That hasn't kept us from trying to do precisely that and the result has always
> been less than satisfactory.  You're introducing more, but I don't think
> there's a reasonable way out of this mess at this point.

 Ack.

> > FWIW I don't understand why the test cases absolutely HAD to have such
> > overlong names guaranteed to exceed our 80 column limit in any context.
> > It's such a pain to handle.
> I dislike the long names as well.  I nearly changed them myself as part of the
> eswin submission, but that seemed a bit gratituous to me so I left them as-is.
> 
> If you wanted to rename them, be my guest, consider it pre-approved ;-)

 Ack.

> WRT the extraneous zero-extension.  Isn't that arguably a bug in the scc
> expander for risc-v?  Fixing that isn't a prerequisite here, but it probably
> worth a bit of someone's time.

 I've looked at it already and it's the middle end that ends up with the 
zero-extension, specifically `convert_move' invoked from `emit_cstore' 
down the call to `noce_try_store_flag_mask', to widen the output from 
`cstoredi4', so I don't think we can do anything in the backend to prevent 
it from happening.  And neither I think we can do anything useful about 
`cstoredi4' having a SImode output, as it's a pattern matched by name 
rather than RTX, so we can't provide variants having a SImode and a DImode 
output each both at a time, as that would cause a name clash.

 What we could do is adding new named patterns with the output mode spelt 
out explicitly, such as `cstoresidi4', `cstoredidi4', etc., but that would 
be quite a big rewrite.  Maybe worth doing, maybe not, I don't know.  For 
the time being the hack I have made does the trick.

  Maciej