Hi Juzhe,
> Currently, we are able to generate step vector with base == 0:
> { 0, 0, 2, 2, 4, 4, ... }
>
> ASM:
>
> vid
> vand
>
> However, we do wrong for step vector with base != 0:
> { 1, 1, 3, 3, 5, 5, ... }
>
> Before this patch, such case will run fail.
>
> After this patch, we are
> Just curious about the combine pass you mentioned, not very sure my
> understand is correct but it looks like the combine pass totally
> ignore the iterator requirement?
>
> It is sort of surprise to me as the combine pass may also need the
> information of iterators.
combine tries to match
> I don't understand why it is necessary to bother "VF". "VF” should
> not be changed since intrinsic stuff is quite stable and any
> unreasonable changes are unacceptable.
Ok, I hear your concern. My argument is: Currently our mechanism
of disabling instructions is incorrect and if any of the
> You change "VF" constraint as "TARGET_ZVFH" which is incorrect since
> we a lot of instructions are valid in "TARGET_ZVFHMIN" in vector.md
> but you disabled them in this patch. You disabled them unexpectedly.
Yes that was kind of the point :) IMHO all the :VF insns are actually
only valid in
Hi,
when working on FP widening/narrowing I realized the Zvfhmin handling
is not ideal right now: We use the "enabled" insn attribute to disable
instructions not available with Zvfhmin (but only with Zvfh).
However, "enabled == 0" only disables insn alternatives, in our case all
of them when
Hi Jivan,
I think Pan is already on this problem. Please see this thread:
https://gcc.gnu.org/pipermail/gcc-patches/2023-June/622129.html
Regards
Robin
Hi,
changes from v1:
- Removed UNSPEC_VNCOPYSIGN
- Adjusted xorsign test expectation.
Regards
Robin
This adds vector copysign, ncopysign and xorsign as well as the
accompanying tests.
gcc/ChangeLog:
* config/riscv/autovec.md (copysign3): Add expander.
(xorsign3): Dito.
Hi Juzhe,
LGTM apart from a tiny nit:
> + /* We have a maximum of 11 operands for RVV instruction patterns according
> to
> + * vector.md. */
> + insn_expander<11> e (/*OP_NUM*/ op_num, /*HAS_DEST_P*/ true,
Seems like you copied this from the non-fp ternary part but the
rest of the file
> You should remove all "unspec" related of "n" ncopysign including
> riscv-vector-builtins-bases.cc
> vector.md/ vector-iterators.md
Ah, there was indeed one stray UNSPEC_VNCOPYSIGN in the iterators, thanks. Any
other
comments before I sent V2?
Regards
Robin
Hi Yanzhang,
while I appreciate the optimization, I'm a bit wary about just adding a special
case for "0". Is that so common? Wouldn't we also like to have
* pow2_p (val) == << val and others?
* 1 should also be covered.
Regards
Robin
Hi,
I'm going to commit the attached. Thanks Lehua for reporting.
Regards
Robin
>From 1a4dfe90f251e38e27104f2fa11feecd3b04c4c1 Mon Sep 17 00:00:00 2001
From: Robin Dapp
Date: Tue, 20 Jun 2023 15:52:16 +0200
Subject: [PATCH] RISC-V: testsuite: Add missing -mabi=lp64d.
This fixes more cases
> By the way, shouldn't these cases have the `-mabi=lp64d` option added,
> otherwise I get the following failure message when I run tests on RV32 GCC.
>
> FAIL: gcc.target/riscv/rvv/autovec/vls-vlmax/vec_set-1.c -std=c99 -O3
> -ftree-vectorize --param riscv-autovec-preference=fixed-vlmax (test
> Could you merge it ?
Committed.
Regards
Robin
> Oh, I should know why. These cases of yours were added yesterday,
> while I submitted the patch the day before, and then yesterday by Pan
> to help me merge in after your cases. Sorry for introducing this issue,
> I'll submit a new fix patch.
Actually they are already in for a bit :)
Hi,
this adds vector copysign, ncopysign and xorsign as well as the
accompanying tests.
In order to easily match the ncopysign patterns I changed the
builtin implementation slightly. Juzhe might want to comment
on that. For now I kept the attribute's name even though
it doesn't emit an "n"
I just noticed there is also a -ffast-math missing in vadd-run.c
as well as one redundant in vrem-rv32gcv.c and added it to the
patch.
Going to commit the attached as obvious.
Regards
Robin
Subject: [PATCH] RISC-V: testsuite: Fix vmul test expectation and fix
-ffast-math.
I forgot to check
> Committed, thanks Jeff.
The vec_set/vec_extract tests FAIL since this commit. I'm going to
commit the attached as obvious.
Lehua, would they not show up in your test runs? You fixed several
other tests but these somehow not?
Regards
Robin
Subject: [PATCH] RISC-V: testsuite: Add -Wno-psabi
> Could you merge it ?
> By the way, could Lehua get the write access?
IMHO nothing stands in the way but I'll defer to Jeff to have
the "official seal" :)
Once he ACKs Lehua needs to go the usual way of requesting
sourceware access via https://sourceware.org/cgi-bin/pdw/ps_form.cgi.
Regards
> This little patch fixes a compile warning issue that my previous
> patch introduced, sorry for introducing this issue.
OK and obvious enough to push directly.
Regards
Robin
LGTM.
Regards
Robin
> Ok. Just sent V2. I will adjust comment and send V3 again :)
Sorry, was too slow.
Regards
Robin
> + /* Step 2: VID AND -NPATTERNS:
> + { 0&-4, 1&-4, 2&-4, 3 &-4, 4 &-4, 5 &-4, 6 &-4, 7 &-4, ... }
> + */
Before that, just add something simple like:
We want to create a pattern where value[ix] = floor (ix / NPATTERNS).
As NPATTERNS is always a power of two we
> This is a nice improvement. Even though we're in the SLP realm I would
> still add an assert that documents that we're indeed operating with
> pow2_p (NPATTERNS) and some comment as to why we can use AND.
> Sure we're doing exact_log2 et al later anyway, just to make things
> clearer.
Actually
Hi,
I forgot to check for vfmul in the multiplication tests. Fix this.
Regards
Robin
gcc/testsuite/ChangeLog:
* gcc.target/riscv/rvv/autovec/binop/vmul-rv32gcv.c: Check for
vfmul.
* gcc.target/riscv/rvv/autovec/binop/vmul-rv64gcv.c: Dito.
---
Hi Juzhe,
> Case 1:
> void
> f (uint8_t *restrict a, uint8_t *restrict b)
> {
> for (int i = 0; i < 100; ++i)
> {
> a[i * 8] = b[i * 8 + 37] + 1;
> a[i * 8 + 1] = b[i * 8 + 37] + 2;
> a[i * 8 + 2] = b[i * 8 + 37] + 3;
> a[i * 8 + 3] = b[i * 8 + 37] + 4;
> a[i *
>> Bootstrap and Regreesion on X86 passed.
>> Jeff and Richi approved.
>>
>> Let's wait for Richard S final approve.
>
> No need to wait.
Thanks, I pushed it in Juzhe's stead, fixing the LEN_LOAD/LEN_STORE
documentation (+ vs -) as r14-1932.
Regards
Robin
>>>If the pattern is not allowed to fail, then what code enforces the bias
>>>argument's restrictions? I don't see it in the generic expander code.
>
> I have no ideal since this is just copied from len_load/len_store which is
> s390 target dependent stuff.
>
> I have sent V7 patch with
Hi,
with the recent changes that we also pass the return value via
stack this is can go forward now.
Changes in V2:
- Remove redundant force_reg.
- Change target selectors to those introduced in the binop patch.
Regards
Robin
This implements the vec_set and vec_extract patterns for integer
Hi,
changes from V2:
- No longer dependent on testsuite changes.
- Add zvfhmin-1.c unary test cases.
Regards
Robin
This patch adds floating-point autovec expanders for vfneg, vfabs as well as
vfsqrt and the accompanying tests.
Similary to the binop tests, there are flavors for zvfh now.
Hi,
changes in v3:
- No longer "dependent" on testsuite changes. Just the zvfh
run testcases use riscv_zvfh_hw, i.e. require that we can compile,
link the code as well as execute the resulting binary.
- Renamed rounding modes (floating_point_rounding_mode feels a
bit long-winded but well...)
> Why do we need '-ffast-math' with the tests?
Normally we would use the COND_ADD to mask out possibly trapping
vector elements and the likes but COND_ADD works with normal
vector masking. What we currently have is no masking but the
LEN_LOAD/LEN_STORE machinery i.e. length-controlled loops.
> This patch would like to fix one maybe-uninitialized warning. Aka:
>
> riscv-vsetvl.cc:4354:3: error: 'vsetvl_rinsn' may be used uninitialized
> [-Werror=maybe-uninitialized]
>
> Signed-off-by: Pan Li
IMHO obvious enough that it doesn't need a maintainer's OK, so go
ahead.
We should make
> <= (operand 2 + operand 4) are used."
Sorry it's really minor (and my mistake) but it should be < and
not <=, right? Mask index 0 is inactive when the length is 0.
> +Perform a masked store (operand 2 + operand 4)
Even more minor but as mentioned the "of" is still missing ;)
Same with
Hi Juzhe,
> +@cindex @code{len_maskload@var{m}@var{n}} instruction pattern
> +@item @samp{len_maskload@var{m}@var{n}}
> +Perform a masked load (operand 2 - operand 4) elements from vector memory
> +operand 1 into vector register operand 0, setting the other elements of
> +operand 0 to undefined
Yes, already did that and will send next version soon.
Just still looking at a lot of test failures with
-march=rv64gc_zvfhmin (note, no rv64gcv) that are
somewhat independent of this patch.
Regards
Robin
Hi,
changes from V1:
- Use VF_AUTO iterator.
- Don't mention vfsqrt7.
This patch adds floating-point autovec expanders for vfneg, vfabs as well as
vfsqrt and the accompanying tests.
Similary to the binop tests, there are flavors for zvfh now.
gcc/ChangeLog:
*
Hi,
changes from V1:
- Add VF_AUTO iterator and use it.
- Ensured we don't ICE with -march=rv64gcv_zfhmin.
this implements the floating-point autovec expanders for binary
operations: vfadd, vfsub, vfdiv, vfmul, vfmax, vfmin and adds
tests.
The existing tests are split up into non-_Float16 and
Hi,
Changes from v1:
- Revamped the target selectors again.
- Fixed some syntax as well as caching errors that were still present.
- Adjusted some test cases I missed.
The current situation with target selectors is improvable at best.
We definitely need to discern between being able to build
> Btw. I'm currently running the testsuite with rv64gcv_zfhmin
> default march and see some additional FAILs. Will report back.
Reporting back - the FAILs are a combination of an older qemu
version and not fully comprehensive target selectors. I'm going
to send a V2 for the testsuite patch as
> the minus in 'operand 2 - operand 3' should be a plus if the
> bias is really zero or -1. I suppose
Yes, that somehow got lost from when the bias was still +1. Maybe
Juzhe can fix this in the course of his patch.
> that's quite conservative. I think you can do better when the
> loads are
On 6/15/23 11:18, Robin Dapp wrote:
>> Meh, PoP is now behind a paywall, trying to get through ... I wonder
>> if there's a nice online html documenting the s390 len_load/store
>> instructions to better understand the need for the bias.
This is z16, but obviously no changes for vll/vstl:
> Meh, PoP is now behind a paywall, trying to get through ... I wonder
> if there's a nice online html documenting the s390 len_load/store
> instructions to better understand the need for the bias.
https://publibfp.dhe.ibm.com/epubs/pdf/a227832c.pdf
Look for vector load with length (store). The
>>> Can you try using the same wording for length and mask operands
>>> as for len_load and maskload? Also len_load has the "bias"
>>> operand which you omit here - IIRC that was added for s390 which
>>> for unknown reason behaves a little different than power. If
>>> len support for s390 ever
Hi Juzhe,
I like the iterator solution better, I added it to the
binops V2 patch with a comment and will post it in a while.
Also realized there is already a testcase and the "enabled"
attribute is set properly now but I hadn't rebased to the
current master branch in a while...
Btw. I'm
Hi,
this patch adds floating-point autovec expanders for vfneg, vfabs as well as
vfsqrt and the accompanying tests. vfrsqrt7 will be added at a later time.
Similary to the binop tests, there are flavors for zvfh now. Prerequisites
as before.
Regards
Robin
gcc/ChangeLog:
*
Hi,
this implements the floating-point autovec expanders for binary
operations: vfadd, vfsub, vfdiv, vfmul, vfmax, vfmin and adds
tests.
The existing tests are amended and split up into non-_Float16
and _Float16 flavors as we cannot rely on the zvfh extension
being present.
As long as we do not
Hi Juzhe,
the general method seems sane and useful (it's not very complicated).
I was just distracted by
> Selector = { 0, 17, 2, 19, 4, 21, 6, 23, 8, 9, 10, 27, 12, 29, 14, 31 }, the
> common expression:
> { 0, nunits + 1, 1, nunits + 2, 2, nunits + 3, ... }
>
> For this selector, we can use
Hi,
this introduces new checks for run tests. Currently we have
riscv_vector as well as rv32 and rv64 which all check if GCC (with the
current configuration) can build (not execute) the respective tests.
Many tests specify e.g. a different -march for vector, though. So the
check fails even
> Oh. I see Robin's email is also wrong. CC Robin too for you
It still arrived via the mailing list ;)
> Good to see a Fix patch of the ICE before Vector ABI patch.
> Let's wait for more comments.
LGTM, this way I don't even need to rewrite my tests.
Regards
Robin
Hi,
> Thanks for fixing this.
>
> This patch let RVV type (both vector and tuple) return in memory by
> default when there is no vector ABI support. It makes sens to me.
>
> CC more RISC-V folks to comments.
so this is intended to fix the PR as well as unblock while we continue
with the
Hi Pan,
> This patch would like to fix one bug exported by RV32 test case
> multiple_rgroup_run-2.c. The mask should be restricted by elen in
> vector, and the condition between the vmv.s.x and the vmv.v.x should
> take inner_bits_size rather than constants.
exported -> exposed.
How about
Hi,
this patch adds the missing (u)int8_t types to the binop tests.
I suggest in the future we have the testsuite pass -march=rv32gcv
as well as -march=rv64gcv as options to each test case instead of
essentially duplicate the files as we do now.
Regards
Robin
gcc/testsuite/ChangeLog:
> I am not sure. These testcases were added by kito long time ago.
> Frankly, I am not familiar with GCC test framework.
Ok, I'm going to have a look. Need to verify the zvfh things anyway.
Regards
Robin
Yes, I agree with the general assessment (and didn't mean to insinuate
that the FAILs are compiler's or a fault of the patch.
> So these 2 failures in RV32 are not the compile's bugs. I have seen:
> /* { dg-do run { target { { {riscv_vector} && {rv64} } } } } */ in
> these testcases which can not
> I don't have a proper sim environment setup yet. How long does the
> testsuite take
> with spike? Have you tried qemu as well?
Any numbers on this Pan? How many cores do you use for running the testsuite?
Regards
Robin
Hi Pan,
these failures were present before the patch I suppose? They
don't look related. Is this what you meant by "the same as upstream"?
> FAIL: gcc.target/riscv/rvv/autovec/vls-vlmax/full-vec-move1.c -std=c99 -O3
> -ftree-vectorize --param riscv-autovec-preference=fixed-vlmax (test for
>
Hi Juzhe,
LGTM. You could also add the aarch64 test disclaimer here again,
but no need for a V2.
Regards
Robin
Hi Juzhe,
thanks, works for me as is. I just hope somebody is going to take on the task
of making different LMUL SLP variants "scannable" at some point because
it would definitely increase our test coverage with these tests. (Or split
the tests manually and not iterate over LMUL)
Regards
Robin
Hi Juzhe,
as the tests are mostly directly from aarch64's testsuite I would
advise comments on where they were taken from as well as a TODO that
they should become common tests for a specific target selector
(vect_scalable_supported or something).
How about some assembly checks for the non-run
> Oh. Sorry. Since I want to commit my patch so I asked Pan to commit
> your test as well. I think you can resend a fix of this testcase and
> drop this patch.
No problem, will fix it another time. Pan can just go ahead with this
fix now, no need to wait for a maintainer, it's obvious enough.
> This patch would like to fix one typo when checking assembly of
> full-vec-movel.
OK. (I actually intended to commit this myself adding some more
comments to the iterator change as well as fix the tests, but well...)
Regards
Robin
> I suggest we implement vector calling convention even though it is not
> ratified yet.
> We can allow calling convention to be enabled only when
> --param=riscv-autovec-preference=fixed-vlmax.
> We have such issue:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110119
>
> + /* If the slide offset fits into 5 bits we can
> + use the immediate variant instead of the register variant.
> + The expander's operand[2] is ops[3] here. */
> + if (!satisfies_constraint_K (ops[3]))
> + ops[3] = force_reg (Pmode, ops[3]);
>
> I don't think we need this.
> Change
>
> +(define_insn "@pred_extract_first_sextdi"
>
> into
>
> (define_insn "*pred_extract_first_sextdi"
Yeah, I was thinking about this as well right after sending.
We will probably never call this directly.
Regards
Robin
Hi,
when the destination register of a vmv.x.s needs to be sign extended to
XLEN we currently emit an sext insn. Since vmv.x.s performs this
implicitly this patch adds two instruction patterns (intended for
combine et al.) that include sign_extend for the destination operand.
The tests extend
Hi,
this implements the vec_set and vec_extract patterns for integer and
floating-point data types. For vec_set we broadcast the insert value to
a vector register and then perform a vslideup with effective length 1 to
the requested index.
vec_extract is done by sliding down the requested
Hi Juzhe,
seems a nice improvement, looks good to me. While reading I was wondering
if vzext could help synthesize some (zero-based) patterns as well
(e.g. 0 3 0 3...).
However the sequences I could come up with were not shorter than what we
are already emitting, so probably not.
Regards
Robin
> + (VNx16QI "TARGET_MIN_VLEN <= 128")
> + (VNx32QI "TARGET_MIN_VLEN <= 256")
> + (VNx64QI "TARGET_MIN_VLEN >= 64 && TARGET_MIN_VLEN <= 512")
> + (VNx128QI "TARGET_MIN_VLEN >= 128 && TARGET_MIN_VLEN <= 1024")
>
> This not correct, we always use VNx16QI as LMUL = m1 for min_vlen >= 128.
>
> +/* We can't enable FP16 NEG/PLUS/MINUS/MULT/DIV auto-vectorization when
> -march="*zvfhmin*". */
> +/* { dg-final { scan-tree-dump-times "vectorized 1 loops in function" 0
> "vect" } } */
Thanks. OK from my side.
Regards
Robin
Hi Juzhe,
no complaints here. Just please make sure you add the commit
message or something related as top comment to the test when
committing.
Somebody who reads the test is not going to want to lookup
the commit message to know what's going on.
Regards
Robin
> I think it shouldn't be with vec_set patch.
> Instead, it obviously should be the separate patch.
Yes, I didn't mean in the actual same patch.
Regards
Robin
On 6/9/23 16:32, juzhe.zh...@rivai.ai wrote:
> From: Juzhe-Zhong
>
> This patch fixes the requirement of V_WHOLE and V_FRACT.
> E.g. VNx8QI in V_WHOLE has no requirement which is incorrect.
> Actually, VNx8QI should be whole(full) mode when TARGET_MIN_VLEN < 128
> since when
> +rtx ops[] = {operands[0], operands[1], operands[2], operands[3]};
> +riscv_vector::emit_vlmax_ternary_insn (code_for_pred_widen_mul_plus
> (, mode),
> +riscv_vector::RVV_WIDEN_TERNOP, ops);
ops is still there ;) No need for another revision
> These enhance patterns are generated in complicate combining situations.
Yes, that's clear. One strategy is to look through combine's output and
see which combination results make sense for a particular backend.
I was wondering where the unspec-less patterns originate (when we
expand
Hi Juzhe,
just one/two really minor nits.
> +rtx ops[] = {operands[0], operands[1], operands[2], operands[3]};
> +riscv_vector::emit_vlmax_ternary_insn (code_for_pred_widen_mul_plus
> (, mode),
> +riscv_vector::RVV_WIDEN_TERNOP, ops);
Here and in
>>> I like the code examples in general but find them hard to read
>>> at lengths > 5-10 or so. Could we condense this a bit?
> Ok, Do I need to send V2 ? Or condense the commit log when merged the patch?
Sure, just condense a bit. No need for V2.
Regards
Robin
Hi Juzhe,
> ...
>vsetvli zero,t1,e8,m1,ta,ma
> vle8.v v1,0(a4)
> vsetvli t3,zero,e16,m2,ta,ma
> vsext.vf2 v6,v1
> vsetvli zero,t1,e8,m1,ta,ma
> vle8.v v1,0(a5)
> vsetvli t3,zero,e16,m2,ta,ma
> add t0,a0,t4
>
Hi Juzhe,
thanks looks pretty comprehensive already.
> +(define_expand "vec_perm"
> + [(match_operand:V 0 "register_operand")
> + (match_operand:V 1 "register_operand")
> + (match_operand:V 2 "register_operand")
> + (match_operand: 3 "vector_perm_operand")]
> + "TARGET_VECTOR &&
Hi Juzhe,
> The approach is quite simple and obvious, changing extension pattern
> into define_insn_and_split will make combine PASS combine into widen
> operations naturally.
looks good to me. Tiny nit: I would add a comment above the patterns
to clarify why insn_and_split instead of expand.
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
>>> but ideally the user would be able to specify -mrvv-size=32 for an
>>> implementation with 32 byte vectors and then vector lowering would make use
>>> of vectors up to 32 bytes?
>
> Actually, we don't want to specify -mrvv-size = 32 to enable vectorization on
> GNU vectors.
> You can take a
Hi Kito,
> GNU vector extensions is widly used around this world, and this patch
> enable that with RISC-V vector extensions, this can help people
> leverage existing code base with RVV, and also can write vector programs in a
> familiar way.
>
> The idea of VLS code gen support is emulate VLS
Hi Juzhe,
>>> Can you explain these two points (3 and 4, maybe 2) a bit in the comments?
>>> I.e. what makes fma different from a normal insn?
> You can take a lookt at vector.md. The ternary instruction pattern has
> operands[0] operands[1] operands[2] operands[3] operands[4] operands[5] :
>
>
Hi Juzhe,
> +;; We can't expand FMA for the following reasons:
But we do :) We just haven't selected the proper alternative yet.
> +;; 1. Before RA, we don't know which multiply-add instruction is the ideal
> one.
> +;;The vmacc is the ideal instruction when operands[3] overlaps
>
> I realize that both TARGET_VECTORIZE_AUTOVECTORIZE_VECTOR_MODES and
> TARGET_VECTORIZE_RELATED_MODE will partially enable some
> auto-vectorization even preferred_simd_mode does not enable
> auto-vectorization when we don't specify
> --param=riscv-autovec-preference.
>
> So plz add
Hi,
> This patch would like to remove the magic number in the riscv-v.cc, and
> align the same value to one macro.
> diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc
> index 458020ce0a1..20b589bf51b 100644
> --- a/gcc/config/riscv/riscv-v.cc
> +++
> Beside, V2 patch should change this:
> emit_vlmax_masked_insn (unsigned icode, int op_num, rtx *ops)
>
> change it into emit_vlmax_masked_mu_insn .
V3 is inline with these changes.
This patch implements abs2, vneg2 and vnot2 expanders
for integer vector registers and adds tests for them.
> I think it's logically incorrect. For ABS, you want:
>
> operands[0] = operads[1] > 0 ? operands[1] : (-operands[1])
> So you should do this following sequence:
>
> vmslt v0,v1,0
> vneg v1,v1v0.t (should use Mask undisturbed)
Yes, this is the emitted sequence, but the vsetvli mask is indeed
Hi,
this patch implements abs2, vneg2 and vnot2 expanders
for integer vector registers and adds tests for them.
v2 is rebased against Juzhe's latest refactoring.
Regards
Robin
gcc/ChangeLog:
* config/riscv/autovec.md (2): Add vneg/vnot.
(abs2): Add.
*
Hi Juzhe,
> use riscv_v_ext_vector_mode_p instead since riscv_v_ext_mode_p includes
> tuple modes.
> You should not use tuple modes in related_mode. Tuple modes will be used in
> array mode target hook and
> used by vec_load_lanes/vec_store_lanes.
Ah, thanks for catching this. Yes,
Hi,
this patch implements the autovec expanders for sign and zero extension
patterns as well as the accompanying truncations. In order to use them
additional mode_attr iterators as well as vectorizer hooks are required.
Using these hooks we can e.g. vectorize with VNx4QImode as base mode
and
>>> Don't you want to use your shiny new operand passing style here as
>>> with the other expanders?
> H, I do this just following ARM code style.
> You can see I do pass rtx[] for expand_vcond and pass rtx,rtx,rtx for
> expand_vec_cmp.
> Well, I just follow ARM SVE implementation (You can
> +(define_expand "vec_cmp"
> + [(set (match_operand: 0 "register_operand")
> + (match_operator: 1 "comparison_operator"
> + [(match_operand:VI 2 "register_operand")
> +(match_operand:VI 3 "register_operand")]))]
> + "TARGET_VECTOR"
> + {
> +riscv_vector::expand_vec_cmp
Hi Juzhe,
thanks, IMHO it's clearer with the changes now. There are still
things that could be improved but it is surely an improvement over
what we currently have. Therefore I'd vote to go ahead so we can
continue with more expanders and changes.
Still, we should be prepared for more
Hi Juzhe,
in general I find the revised structure quite logical and it is definitely
an improvement. Some abstraction are still a bit leaky but we can always
refactor "on the fly". Some comments on the general parts, skipping
over the later details.
> bool m_has_dest_p;
Why does a store not
> I do refactoring since we are going to have many different
> auto-vectorization patterns, for example: cond_addetc.
>
> I should make the current framework suitable for all of them to
> simplify the future work.
That's good in general but can't it wait until the respective
changes go in?
> Thanks Robin. Address comment.
Did you intend to send an update here already or are you working
on it? Just wondering because you just sent another refactoring
patch.
Regards
Robin
> So I expect you will also apply those refactor on Juzhe's new changes?
> If so I would like to have a separated NFC refactor patch if possible.
What's NFC? :) Do you mean to just have the refactor part as a separate
patch? If yes, I agree.
> e.g.
> Juzhe's vec_cmp/vcond -> NFC refactor patch
As discussed with Juzhe off-list, I will rebase this patch against
Juzhe's vec_cmp/vcond patch once that hits the trunk.
Regards
Robin
Hi Juzhe,
thanks. Some remarks inline.
> +;; Integer (signed) vcond. Don't enforce an immediate range here, since it
> +;; depends on the comparison; leave it to riscv_vector::expand_vcond instead.
> +(define_expand "vcond"
> + [(set (match_operand:V 0 "register_operand")
> +
201 - 300 of 460 matches
Mail list logo