> Date: Fri, 29 Sep 2023 16:37:21 -0600 > From: Jeff Law <jeffreya...@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
