On 8/23/23 18:42, Vineet Gupta wrote:
Seriously, I detest it too, but the irony is I've now made my 2nd change
in there and keep adding to ugliness :-(
Happens to all of us sometimes.
So I think your change makes sense. But I think it can be refined to
simplify the larger chunk of code we're looking at:
/* If the constant is likely to be stored in a GPR, SETs of
single-insn constants are as cheap as register sets; we
never want to CSE them. */
if (cost == 1 && outer_code == SET)
*total = 0;
/* When we load a constant more than once, it usually is
better
to duplicate the last operation in the sequence than to CSE
the constant itself. */
else if (outer_code == SET || GET_MODE (x) == VOIDmode)
*total = COSTS_N_INSNS (1);
Turns into
if (outer_code == SET || GET_MODE (x) == VOIDmode)
*total = COSTS_N_INSNS (1);
Yep that's what I started with too but then left it, leaving it as an
visual indication to fix things up when ultimately cost model returns
the actual num of insns for a non trivial large const.Leaving the code
there meant we But I agree I'll fold it and add a TODO comment for
improving the cost model.
For the current proposal I do want to understand/reason what is left
there - what cases are we trying to filter out with #2467 ?
| case CONST:
| if ((cost = riscv_const_insns (x)) > 0) # 2465
| {
| if (outer_code == SET || GET_MODE (x) == VOIDmode) # 2467
| *total = COSTS_N_INSNS
(1); # 2468
(1) AFAIU, VOIDmode is for const_int - and supposedly true for symbolic
addresses etc whose actual values are not known at compile time ? Or is
it needed just as an artifact of the weird fall through.
I'd expect it to filter away some symbolics and perhaps floating point
constants.
(2) outer_code SET will kick in for set_src_cost( ) call from Hoist,
which passes a const_int rtx directly.
But in case of say expand_mult () -> set_src_cost ( ) called for say
(mult:DI (reg:DI 134)
(const_int [0x202020202020202]))
In the eventual call for const_int operand, outer_code is MULT,
so we elide #2468
But wait we don't even hit #2467 since #2465 has a weird cap
inside which caps 6 to 0.
| case CONST_INT:
| {
| int cost = riscv_integer_cost (INTVAL (x));
| /* Force complicated constants to memory. */
| return cost < 4 ? cost : 0; #1321
| }
This definitely needs to be tracked separately in a PR
And when we solve it, it'll likely need to be uarch driven. I've hinted
before that there'll be changes in this area that will allow some
targets to handle most, if not all, of those complicated constants in a
single cycle.
With a suitable comment about GCSE and the general desire to duplicate
the last op rather than CSE the constant for multi instruction
constant synthesis cases.
Hmm, do we not prefer GCSE/CSE a 3-4 insn const sequence. It seems the
RA is capable enough of undoing that ;-)
For now I can I can keep the comment with current philosophy
I mentally set aside the multi-insn sequence concern. Presumably the
code was written that way for a reason and without a good one, backed by
data, I was hesitant to push back on that choice.
If you agree, then consider the patch pre-approved with that change.
If not, then state why and your original patch is OK as well.
I'm afraid I have more questions than either of us were hoping for :-)
But it seems we can chunk up the work for just Hoist enabling and then
improve the const cost model with that new PR.
I'll spin up a buildroot testbed to get a wider impact of that change.
Exactly. THere's multiple issues, but I think they can be tackled
independently.
jeff
