I agree with Andrew.
And I don't think this patch is appropriate for following reasons:
1. This patch increases vector workload in machine since
it convert scalar load + vmv.v.x into vmv.v.i + vsll.vi.
2. For multi-issue OoO machine, scalar instructions are very cheap
when they are located in vector codegen. For example a sequence
like this:
scalar insn
scalar insn
vector insn
scalar insn
vector insn
In such situation, we can issue multiple instructions simultaneously,
and the latency of scalar instructions will be hided so scalar instruction
is cheap. Wheras this patch increasing vector pipeline workload is not
friendly to OoO machine what I mentioned above.
3. I can image the only benefit of this patch is that we can reduce scalar
register pressure
in some extreme circumstances. However, I don't this benefit is "real"
since GCC should
well schedule the instruction sequence when we well tune the vector
instructions scheduling
model and cost model to make such register live range very short when the
scalar register
pressure is very high.
Overal, I disagree with this patch.
Thanks.
juzhe.zh...@rivai.ai
From: Andrew Waterman
Date: 2023-05-31 04:18
To: Robin Dapp
CC: gcc-patches; Kito Cheng; palmer; juzhe.zh...@rivai.ai; jeffreyalaw
Subject: Re: [PATCH] RISC-V: Synthesize power-of-two constants.
This turns out to be a de-optimization for implementations with any
amount of temporal execution (which is most machines with LMUL > 1 and
even some machines with LMUL <= 1). Scalar instructions are generally
cheaper than multi-cycle-occupancy vector operations, so reducing
scalar work by increasing vector work is normally not a good tradeoff.
(And even if the vector instruction has unit occupancy, it likely
burns a bit more energy.) The best generic scheme to load 143 into
all elements of a vector register is to first load 143 into a scalar
register, then use vmv.v.x. If the proposed scheme is profitable on
some implementations in some circumstances, it should probably be
enabled only when tuning for that implementation.
On Tue, May 30, 2023 at 12:14 PM Robin Dapp via Gcc-patches
wrote:
>
> Hi,
>
> I figured I'd send this patch that I quickly hacked together some
> days back. It's likely going to be controversial because we don't
> have vector costs in place at all yet and even with costs it's
> probably debatable as the emitted sequence is longer :)
> I'm willing to defer or ditch it altogether but as it's small and
> localized why not at least discuss it quickly.
>
> For immediates that are powers of two, instead of loading them into a
> GPR and then broadcasting (incurring the scalar-vector latency) we
> can synthesize them with a vmv.vi and a vsll.v.i. Depending on actual
> costs we could also add more complicated synthesis patterns in the
> future.
>
> Regards
> Robin
>
> gcc/ChangeLog:
>
> * config/riscv/riscv-selftests.cc (run_const_vector_selftests):
> Adjust expectation.
> * config/riscv/riscv-v.cc (expand_const_vector): Synthesize
> power-of-two constants.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/riscv/rvv/autovec/vmv-imm-fixed-rv32.c: Adjust test
> expectation.
> * gcc.target/riscv/rvv/autovec/vmv-imm-fixed-rv64.c: Dito.
> * gcc.target/riscv/rvv/autovec/vmv-imm-rv32.c: Dito.
> * gcc.target/riscv/rvv/autovec/vmv-imm-rv64.c: Dito.
> ---
> gcc/config/riscv/riscv-selftests.cc | 9 +-
> gcc/config/riscv/riscv-v.cc | 31 +++
> .../riscv/rvv/autovec/vmv-imm-fixed-rv32.c| 5 +--
> .../riscv/rvv/autovec/vmv-imm-fixed-rv64.c| 5 +--
> .../riscv/rvv/autovec/vmv-imm-rv32.c | 5 +--
> .../riscv/rvv/autovec/vmv-imm-rv64.c | 5 +--
> 6 files changed, 51 insertions(+), 9 deletions(-)
>
> diff --git a/gcc/config/riscv/riscv-selftests.cc
> b/gcc/config/riscv/riscv-selftests.cc
> index 1bf1a648fa1..21fa460bb1f 100644
> --- a/gcc/config/riscv/riscv-selftests.cc
> +++ b/gcc/config/riscv/riscv-selftests.cc
> @@ -259,9 +259,16 @@ run_const_vector_selftests (void)
> rtx_insn *insn = get_last_insn ();
> rtx src = XEXP (SET_SRC (PATTERN (insn)), 1);
> /* 1. Should be vmv.v.i for in rang of -16 ~ 15.
> -2. Should be vmv.v.x for exceed -16 ~ 15. */
> +2. For 16 (and appropriate higher powers of two)
> + expect a shift because we emit a
> + vmv.v.i v1, 8 and a
> + vsll.v.i v1, v1, 1.
> +3. Should be vmv.v.x for everything else. */
> if (IN_RANGE (val, -16, 15))
> ASSERT_TRUE (rtx_equal_p (src, dup));
> + else if (IN_RANGE (val, 16, 16))
> + ASSERT_TRUE (GET_CODE (src) == ASHIFT
> +&& INTVAL (XEXP (src, 1)) == 1);
>
