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

[Jeff Law <jeffreyalaw@gmail.com>]

On 11/9/23 07:33, Maciej W. Rozycki wrote:
> 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?
FYI, I've opened a bug for this issue so it doesn't get lost.  I don't 
think the extensions are terribly hard.  It's really a matter of 
deciding if we can re-use any of the logic from the expander or if we 
just mirror its logic and keep the expander and costing in sync.

Jeff
> 
>    Maciej