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?
What needs to happen is that code needs to be extended, not reverted. 
Many codes have to be synthesized based on the condition and the 
true/false arms.  That's not currently accounted for.


jeff