Re: [committed] RISC-V: Fix INSN costing and more zicond tests

[Hans-Peter Nilsson <hp@axis.com>]

> Date: Fri, 29 Sep 2023 16:37:21 -0600
> From: Jeff Law <jeffreyalaw@gmail.com>

> So this ends up looking a lot like the bits that I had to revert several 
> weeks ago :-)
> 
> The core issue we have is given an INSN the generic code will cost the 
> SET_SRC and SET_DEST and sum them.  But that's far from ideal on a RISC 
> target.
> 
> For a register destination, the cost can be determined be looking at 
> just the SET_SRC.  Which is precisely what this patch does.  When the 
> outer code is an INSN and we're presented with a SET we take one of two 
> paths.
> 
> If the destination is a register, then we recurse just on the SET_SRC 
> and we're done.  Otherwise we fall back to the existing code which sums 
> the cost of the SET_SRC and SET_DEST.

Ackchyually...  that "otherwise" happens for calls to
set_rtx_cost (et al), but not calls to insn_cost.

IOW, with that patch, it seems you're mimicking insn_cost
behavior also for set_rtx_cost (et al).  You're likely aware
of this, but when seeing these target cost functions tweaked
for reasons that appear somewhat empirical, I felt compelled
to point out the related rabbit-hole.

While I'm ranting, these slightly different cost api:s,
somewhat arbitrarily, (or empirically) picked by callers, is
a problem by itself.  Not to mention that the default use of
set_rtx_cost means you get hit by another problem; the
default cost of 0 for registers is also a magic number to
pattern_cost to set the cost to INSN_COSTS (1).

The default insn_cost implementation, which RISC-V uses as
opposed to implementing the TARGET_INSN_COST hook, only
looks at the SET_SRC for calls to insn_cost for single-sets.
See pattern_cost.  I believe that's a bug.  Fixing that was
attempted in 2016 (by Bernd S.), a patch which was later
reverted: cf. commits r7-4866-g334442f282a9d6 and
r7-4930-g03612f25277590.  Hence rabbit-hole.  (And no,
implementing TARGET_INSN_COST doesn't automatically fix
things.  Too much of the gcc middle-end appears tuned to the
default behavior.)

Sorry for the rant; have a nice day and a better week-end.

>  That fallback path isn't great 
> and probably could be further improved (just costing SET_DEST in that 
> case is probably quite reasonable).
> 
> The difference between this version and the bits that slipped through by 
> accident several weeks ago is that old version mis-used the API due to a 
> thinko on my part.
> 
> This tightens up various zicond tests to avoid undesirable matching.
> 
> This has been tested on rv64gc -- the only difference it makes on the 
> testsuite is the new tests (included in this patch) flip from failing to 
> passing.
> 
> Pushed to the trunk.
> 
> Jeff

brgds, H-P