>> Looks like just a line-break change and the line is not too long?
Yes.
>> This seems a bit inconsistent from a caller's perspective
>> as we also do emit_insn (gen_vec_series, ...) without extra move
>> at another spot. Can we handle this directly in expand_vec_series?
I'am not sure. I change this because I encounter an issue in my tests.
Could you try this patch without this change and run tests?
I am not whether this patch is the only correct fix.
>>This hunk seems unrelated to the rest. I suppose it's just a fixup
>>for 1-element float vectors for VLS?
No, this is related since there will be an ICE when I didn't add these modes.
Could you try this? I didn't spend time on the details of this.
juzhe.zh...@rivai.ai
From: Robin Dapp
Date: 2023-08-08 20:50
To: Juzhe-Zhong; gcc-patches
CC: rdapp.gcc; kito.cheng; kito.cheng; jeffreyalaw
Subject: Re: [PATCH] RISC-V: Allow CONST_VECTOR for VLS modes.
Hi Juzhe,
just some nits.
> - else if (rtx_equal_p (step, constm1_rtx) && poly_int_rtx_p (base, &value)
> + else if (rtx_equal_p (step, constm1_rtx)
> + && poly_int_rtx_p (base, &value)
Looks like just a line-break change and the line is not too long?
> - rtx ops[] = {dest, vid, gen_int_mode (nunits_m1, GET_MODE_INNER
> (mode))};
> - insn_code icode = code_for_pred_sub_reverse_scalar (mode);
> - emit_vlmax_insn (icode, RVV_BINOP, ops);
> + if (value.is_constant () && IN_RANGE (value.to_constant (), -16, 15))
At some point, we'd want to unify all the [-16, 15] handling. We already have
simm5_p but that takes an rtx. Not urgent for now just to keep in mind.
> + {
> + rtx dup = gen_const_vector_dup (mode, value);
> + rtx ops[] = {dest, dup, vid};
> + insn_code icode = code_for_pred (MINUS, mode);
> + emit_vlmax_insn (icode, RVV_BINOP, ops);
> + }
> + else
> + {
> + rtx ops[]
> + = {dest, vid, gen_int_mode (nunits_m1, GET_MODE_INNER (mode))};
> + insn_code icode = code_for_pred_sub_reverse_scalar (mode);
> + emit_vlmax_insn (icode, RVV_BINOP, ops);
> + }
> return;
> }
> else
> @@ -1416,7 +1428,9 @@ expand_const_vector (rtx target, rtx src)
> rtx base, step;
> if (const_vec_series_p (src, &base, &step))
> {
> - emit_insn (gen_vec_series (mode, target, base, step));
> + rtx tmp = gen_reg_rtx (mode);
> + emit_insn (gen_vec_series (mode, tmp, base, step));
> + emit_move_insn (target, tmp);
This seems a bit inconsistent from a caller's perspective
as we also do emit_insn (gen_vec_series, ...) without extra move
at another spot. Can we handle this directly in expand_vec_series?
> + (V1HF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_16")
> (V2HF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_16")
> (V4HF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_16")
> (V8HF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_16")
> @@ -479,6 +480,7 @@
> (V512HF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_16 && TARGET_MIN_VLEN
> >= 1024")
> (V1024HF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_16 && TARGET_MIN_VLEN
> >= 2048")
> (V2048HF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_16 && TARGET_MIN_VLEN
> >= 4096")
> + (V1SF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_32")
> (V2SF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_32")
> (V4SF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_32")
> (V8SF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_32")
> @@ -489,6 +491,7 @@
> (V256SF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_32 && TARGET_MIN_VLEN
> >= 1024")
> (V512SF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_32 && TARGET_MIN_VLEN
> >= 2048")
> (V1024SF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_32 && TARGET_MIN_VLEN
> >= 4096")
> + (V1DF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_64")
> (V2DF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_64")
> (V4DF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_64")
> (V8DF "TARGET_VECTOR_VLS && TARGET_VECTOR_ELEN_FP_64 && TARGET_MIN_VLEN >=
> 64")
This hunk seems unrelated to the rest. I suppose it's just a fixup
for 1-element float vectors for VLS?
Apart from that, looks good to me.
Regards
Robin
