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
