On 8/9/23 18:30, Vineet Gupta wrote:
Hoist want_to_gcse_p () calls rtx_cost () to compute max distance for
hoist candidates . For a const with cost 1 backend currently returns 0,
causing Hoist to bail and elide GCSE.
Note that constants requiring more than 1 insns to setup were working
already since backend is returning 1 as well. Arguably that needs updating
as well to reflect cost better, but that should be a different change
anyways.
To keep testsuite parity, some V tests need updatinge which started failing
in the new costing regime.
gcc/ChangeLog:
* gcc/config/riscv.cc (riscv_rtx_costs): Adjust const_int cost.
gcc/testsuite/ChangeLog:
* gcc.target/riscv/gcse-const.c: New Test
* gcc.target/riscv/rvv/vsetvl/vlmax_conflict-7.c: Disable for
-O2.
* gcc.target/riscv/rvv/vsetvl/vlmax_conflict-8.c: Ditto.
Thanks for your patience on this. I needed a bit of time to gather my
thoughts and review some code.
index 8b7256108157..1802eef908fc 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -2464,14 +2464,9 @@ riscv_rtx_costs (rtx x, machine_mode mode, int
outer_code, int opno ATTRIBUTE_UN
case CONST:
if ((cost = riscv_const_insns (x)) > 0)
{
- /* 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. */
+ /* Hoist will GCSE constants only if cost is non-zero. */
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. */
+ *total = COSTS_N_INSNS (1);
else if (outer_code == SET || GET_MODE (x) == VOIDmode)
*total = COSTS_N_INSNS (1);
}
So the concern here was we have two classes of constants which can be
synthesized in a single instruction.
One class would be those constants that can be used as-is in most
instructions. (const_int 0) being the most obvious, but of course
there's many others.
The other class can be synthesized in a single instruction, but aren't
typically usable in something like addi, andi, etc. A good example
might be (const_int 0x40000000).
I wanted to make sure we were doing something sensible across those two
cases. And I think we probably are as we have an earlier check in the
CONST_INT case (no I don't like the case fallthrus at all :(
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);
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.
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.
Again, thanks for your patience on this.
jeff
