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

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

On Fri, 29 Sep 2023, Jeff Law wrote:

> 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.  That fallback path isn't great and probably
> could be further improved (just costing SET_DEST in that case is probably
> quite reasonable).

 So this actually breaks insn costing for if-conversion, causing all 
conditional-move expansions to count as 1 insn regardless of how many 
there actually are.  This can be easily verified by using various 
`-mbranch-cost=' settings.

 Before your change you had to set the branch cost to higher than or equal
to the replacement insn count for if-conversion to trigger.  Of course 
tuning microarchitectures will have preset this hopefully correctly for 
their needs, so normally you don't need to resort to `-mbranch-cost='.  
With this change in place only setting `-mbranch-cost=1' will prevent 
if-conversion from triggering, which is taking the situation back to GCC 
13 days, where `movMODEcc' patterns were always cost at 1.

 In preparation for an upcoming set of changes I have written numerous 
testsuite cases to verify this insn costing to work correctly and now that 
I have rebased for the submission all indicate the costing went wrong and 
`movMODEcc' sequences of up to 6 insns are all now cost at 1 total.  I was 
going to post the patch series Fri-Mon, but this seems like a showstopper 
to me, because if-conversion now triggers even when the conditional-move 
(or for that matter conditional-add, as I have it handled too) sequence is 
more expensive than a branched one.

 E.g. the NE operation costs 4 instructions for Zicond:

	sub	a1,a0,a1
	czero.eqz	a2,a2,a1
	czero.nez	a1,a3,a1
	or	a0,a2,a1
	ret

while the branched equivalent costs (branch + 1) instructions:

	beq	a0,a1,.L3
	mv	a0,a2
	ret
.L3:
	mv	a0,a3
	ret

so I'd expect if-conversion only to trigger at `-mbranch-cost=3' or higher 
(just as my test cases verify), but now it triggers at `-mbranch-cost=2' 
already.

 Can we have the insn costing reverted to correct calculation?

  Maciej