Message ID | 20180126045742.5487-1-richard.henderson@linaro.org |
---|---|
Headers | show |
Series | tcg: generic vector operations | expand |
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20180126045742.5487-1-richard.henderson@linaro.org Subject: [Qemu-devel] [PATCH v11 00/20] tcg: generic vector operations === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu t [tag update] patchew/cover.1516978645.git.berto@igalia.com -> patchew/cover.1516978645.git.berto@igalia.com Switched to a new branch 'test' 4c8653a236 tcg/aarch64: Add vector operations fdd44d797c tcg/i386: Add vector operations c577c94241 target/arm: Use vector infrastructure for aa64 orr/bic immediate 04334650eb target/arm: Use vector infrastructure for aa64 multiplies e2c680b3d6 target/arm: Use vector infrastructure for aa64 compares eb96f545c4 target/arm: Use vector infrastructure for aa64 constant shifts d2b9095aa0 target/arm: Use vector infrastructure for aa64 dup/movi 08cab5a4b7 target/arm: Use vector infrastructure for aa64 mov/not/neg 63eed90fe5 target/arm: Use vector infrastructure for aa64 add/sub/logic cdeae1b2f4 target/arm: Align vector registers a8f6fc0513 tcg/optimize: Handle vector opcodes during optimize 236c1e791e tcg: Add generic vector helpers with a scalar operand 39f63ea046 tcg: Add generic helpers for saturating arithmetic 46f37394ad tcg: Add generic vector ops for multiplication 9ab0c8a296 tcg: Add generic vector ops for comparisons c29fe95be6 tcg: Add generic vector ops for constant shifts 686bb152ff tcg: Add generic vector expanders bfbabddcfc tcg: Standardize integral arguments to expanders 2c5e460f34 tcg: Add types and basic operations for host vectors 14ab7b00be tcg: Allow multiple word entries into the constant pool === OUTPUT BEGIN === Checking PATCH 1/20: tcg: Allow multiple word entries into the constant pool... Checking PATCH 2/20: tcg: Add types and basic operations for host vectors... ERROR: externs should be avoided in .c files #123: FILE: tcg/tcg-op-vec.c:32: +extern TCGv_i32 TCGV_LOW_link_error(TCGv_i64); ERROR: externs should be avoided in .c files #124: FILE: tcg/tcg-op-vec.c:33: +extern TCGv_i32 TCGV_HIGH_link_error(TCGv_i64); ERROR: Macros with complex values should be enclosed in parenthesis #453: FILE: tcg/tcg-opc.h:209: +#define IMPLVEC TCG_OPF_VECTOR | IMPL(TCG_TARGET_MAYBE_vec) ERROR: Macros with complex values should be enclosed in parenthesis #713: FILE: tcg/tcg.h:621: +#define TCGOP_VECL(X) (X)->param1 ERROR: Macros with complex values should be enclosed in parenthesis #714: FILE: tcg/tcg.h:622: +#define TCGOP_VECE(X) (X)->param2 total: 5 errors, 0 warnings, 720 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 3/20: tcg: Standardize integral arguments to expanders... Checking PATCH 4/20: tcg: Add generic vector expanders... ERROR: spaces required around that '&' (ctx:WxO) #350: FILE: accel/tcg/tcg-runtime-gvec.c:311: + *(vec64 *)(d + i) = *(vec64 *)(a + i) &~ *(vec64 *)(b + i); ^ ERROR: space prohibited after that '~' (ctx:OxW) #350: FILE: accel/tcg/tcg-runtime-gvec.c:311: + *(vec64 *)(d + i) = *(vec64 *)(a + i) &~ *(vec64 *)(b + i); ^ ERROR: spaces required around that '|' (ctx:WxO) #361: FILE: accel/tcg/tcg-runtime-gvec.c:322: + *(vec64 *)(d + i) = *(vec64 *)(a + i) |~ *(vec64 *)(b + i); ^ ERROR: space prohibited after that '~' (ctx:OxW) #361: FILE: accel/tcg/tcg-runtime-gvec.c:322: + *(vec64 *)(d + i) = *(vec64 *)(a + i) |~ *(vec64 *)(b + i); ^ ERROR: trailing whitespace #992: FILE: tcg/tcg-op-gvec.c:464: + } $ ERROR: space prohibited after that open parenthesis '(' #2234: FILE: tcg/tcg.h:1230: + ? ( (VECE) == MO_8 ? 0x0101010101010101ull * (uint8_t)(C) \ total: 6 errors, 0 warnings, 2158 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 5/20: tcg: Add generic vector ops for constant shifts... Checking PATCH 6/20: tcg: Add generic vector ops for comparisons... ERROR: spaces required around that '*' (ctx:WxV) #32: FILE: accel/tcg/tcg-runtime-gvec.c:485: + *(TYPE *)(d + i) = DO_CMP0(*(TYPE *)(a + i) OP *(TYPE *)(b + i)); \ ^ ERROR: spaces required around that '==' (ctx:WxB) #38: FILE: accel/tcg/tcg-runtime-gvec.c:491: + DO_CMP1(gvec_eq##SZ, vec##SZ, ==) \ ^ ERROR: spaces required around that '!=' (ctx:WxB) #39: FILE: accel/tcg/tcg-runtime-gvec.c:492: + DO_CMP1(gvec_ne##SZ, vec##SZ, !=) \ ^ ERROR: spaces required around that '<' (ctx:WxB) #40: FILE: accel/tcg/tcg-runtime-gvec.c:493: + DO_CMP1(gvec_lt##SZ, svec##SZ, <) \ ^ ERROR: spaces required around that '<=' (ctx:WxB) #41: FILE: accel/tcg/tcg-runtime-gvec.c:494: + DO_CMP1(gvec_le##SZ, svec##SZ, <=) \ ^ ERROR: spaces required around that '<' (ctx:WxB) #42: FILE: accel/tcg/tcg-runtime-gvec.c:495: + DO_CMP1(gvec_ltu##SZ, vec##SZ, <) \ ^ ERROR: spaces required around that '<=' (ctx:WxB) #43: FILE: accel/tcg/tcg-runtime-gvec.c:496: + DO_CMP1(gvec_leu##SZ, vec##SZ, <=) ^ ERROR: space prohibited after that '&&' (ctx:ExW) #238: FILE: tcg/tcg-op-gvec.c:1709: + && check_size_impl(oprsz, 8) ^ total: 8 errors, 0 warnings, 303 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 7/20: tcg: Add generic vector ops for multiplication... Checking PATCH 8/20: tcg: Add generic helpers for saturating arithmetic... ERROR: spaces required around that '&' (ctx:WxO) #64: FILE: accel/tcg/tcg-runtime-gvec.c:594: + if (((di ^ ai) &~ (ai ^ bi)) < 0) { ^ ERROR: space prohibited after that '~' (ctx:OxW) #64: FILE: accel/tcg/tcg-runtime-gvec.c:594: + if (((di ^ ai) &~ (ai ^ bi)) < 0) { ^ ERROR: spaces required around that '&' (ctx:WxO) #82: FILE: accel/tcg/tcg-runtime-gvec.c:612: + if (((di ^ ai) &~ (ai ^ bi)) < 0) { ^ ERROR: space prohibited after that '~' (ctx:OxW) #82: FILE: accel/tcg/tcg-runtime-gvec.c:612: + if (((di ^ ai) &~ (ai ^ bi)) < 0) { ^ total: 4 errors, 0 warnings, 411 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 9/20: tcg: Add generic vector helpers with a scalar operand... Checking PATCH 10/20: tcg/optimize: Handle vector opcodes during optimize... ERROR: spaces required around that ':' (ctx:VxE) #170: FILE: tcg/optimize.c:644: + CASE_OP_32_64_VEC(add): ^ ERROR: spaces required around that ':' (ctx:VxE) #171: FILE: tcg/optimize.c:645: + CASE_OP_32_64_VEC(mul): ^ ERROR: spaces required around that ':' (ctx:VxE) #172: FILE: tcg/optimize.c:646: + CASE_OP_32_64_VEC(and): ^ ERROR: spaces required around that ':' (ctx:VxE) #173: FILE: tcg/optimize.c:647: + CASE_OP_32_64_VEC(or): ^ ERROR: spaces required around that ':' (ctx:VxE) #174: FILE: tcg/optimize.c:648: + CASE_OP_32_64_VEC(xor): ^ ERROR: spaces required around that ':' (ctx:VxE) #183: FILE: tcg/optimize.c:713: + CASE_OP_32_64_VEC(sub): ^ ERROR: spaces required around that ':' (ctx:VxE) #206: FILE: tcg/optimize.c:744: + CASE_OP_32_64_VEC(xor): ^ ERROR: spaces required around that ':' (ctx:VxE) #215: FILE: tcg/optimize.c:761: + CASE_OP_32_64_VEC(andc): ^ ERROR: spaces required around that ':' (ctx:VxE) #224: FILE: tcg/optimize.c:769: + CASE_OP_32_64_VEC(orc): ^ ERROR: spaces required around that ':' (ctx:VxE) #246: FILE: tcg/optimize.c:807: + CASE_OP_32_64_VEC(add): ^ ERROR: spaces required around that ':' (ctx:VxE) #247: FILE: tcg/optimize.c:808: + CASE_OP_32_64_VEC(sub): ^ ERROR: spaces required around that ':' (ctx:VxE) #248: FILE: tcg/optimize.c:809: + CASE_OP_32_64_VEC(or): ^ ERROR: spaces required around that ':' (ctx:VxE) #249: FILE: tcg/optimize.c:810: + CASE_OP_32_64_VEC(xor): ^ ERROR: spaces required around that ':' (ctx:VxE) #250: FILE: tcg/optimize.c:811: + CASE_OP_32_64_VEC(andc): ^ ERROR: spaces required around that ':' (ctx:VxE) #268: FILE: tcg/optimize.c:824: + CASE_OP_32_64_VEC(and): ^ ERROR: spaces required around that ':' (ctx:VxE) #269: FILE: tcg/optimize.c:825: + CASE_OP_32_64_VEC(orc): ^ ERROR: spaces required around that ':' (ctx:VxE) #279: FILE: tcg/optimize.c:1039: + CASE_OP_32_64_VEC(and): ^ ERROR: spaces required around that ':' (ctx:VxE) #280: FILE: tcg/optimize.c:1040: + CASE_OP_32_64_VEC(mul): ^ ERROR: spaces required around that ':' (ctx:VxE) #290: FILE: tcg/optimize.c:1055: + CASE_OP_32_64_VEC(or): ^ ERROR: spaces required around that ':' (ctx:VxE) #291: FILE: tcg/optimize.c:1056: + CASE_OP_32_64_VEC(and): ^ ERROR: spaces required around that ':' (ctx:VxE) #302: FILE: tcg/optimize.c:1068: + CASE_OP_32_64_VEC(andc): ^ ERROR: spaces required around that ':' (ctx:VxE) #303: FILE: tcg/optimize.c:1069: + CASE_OP_32_64_VEC(sub): ^ ERROR: spaces required around that ':' (ctx:VxE) #304: FILE: tcg/optimize.c:1070: + CASE_OP_32_64_VEC(xor): ^ ERROR: spaces required around that ':' (ctx:VxE) #313: FILE: tcg/optimize.c:1084: + CASE_OP_32_64_VEC(mov): ^ total: 24 errors, 0 warnings, 296 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 11/20: target/arm: Align vector registers... Checking PATCH 12/20: target/arm: Use vector infrastructure for aa64 add/sub/logic... Checking PATCH 13/20: target/arm: Use vector infrastructure for aa64 mov/not/neg... Checking PATCH 14/20: target/arm: Use vector infrastructure for aa64 dup/movi... Checking PATCH 15/20: target/arm: Use vector infrastructure for aa64 constant shifts... Checking PATCH 16/20: target/arm: Use vector infrastructure for aa64 compares... Checking PATCH 17/20: target/arm: Use vector infrastructure for aa64 multiplies... Checking PATCH 18/20: target/arm: Use vector infrastructure for aa64 orr/bic immediate... Checking PATCH 19/20: tcg/i386: Add vector operations... Checking PATCH 20/20: tcg/aarch64: Add vector operations... === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@freelists.org
Richard Henderson <richard.henderson@linaro.org> writes: > Changes since v11: > * Use dup_const more. > * Cleanup some gvec 2i and 2s routines. > * Use more helpers and less gotos in target/arm/translate-a64.c. I think this series is good to go. A quick word on performance. I saw a slight dip for the string sort in Emilio's dbt-bench/nbench: https://i.imgur.com/K5AFr1u.png And: NBench score; higher is better 140 +-+-----+------+-------+-------+------+-------+-------+------+-----+-+ | **** | | * *## development | 120 +-+.........................*..*.#....................master.......+-+ | * * # | 100 +-+............####.........*..*.#.................................+-+ | # # * * # | | *** # * * # | 80 +-+..........*.*..#.........*..*.#.........****###.................+-+ | * * # * * # * * # | 60 +-+..........*.*..#.........*..*.#..***###.*..*..#.........***###..+-+ | * * # ****### * * # * * # * * # * * # | | * * # * * # * * # * * # * * # ****## * * # | 40 +-+..........*.*..#.*..*..#.*..*.#..*.*..#.*..*..#.*..*.#..*.*..#..+-+ | * * # * * # * * # * * # * * # * * # * * # | 20 +-+..........*.*..#.*..*..#.*..*.#..*.*..#.*..*..#.*..*.#..*.*..#..+-+ | ****## * * # * * # * * # * * # * * # * * # * * # | | * * # * * # * * # * * # * * # * * # * * # * * # | 0 +-+--****##--***###-****###-****##--***###-****###-****##--***###--+-+ NUMERIC STRING SOBITFIEFP EMULAASSIGNMENT IDEA HUFFMAN gmean We think this is likely the strajust function which hits a loop utilising a single vector. We already know a single vector-op is a worse case given the latency but this improves if the code is -funrolled or ultimately re-built with support for bigger vectors ;-) I certainly don't think it's a blocker to merging given the other benchmarks look pretty good including slight wins on others. > > > Richard Henderson (20): > tcg: Allow multiple word entries into the constant pool > tcg: Add types and basic operations for host vectors > tcg: Standardize integral arguments to expanders > tcg: Add generic vector expanders > tcg: Add generic vector ops for constant shifts > tcg: Add generic vector ops for comparisons > tcg: Add generic vector ops for multiplication > tcg: Add generic helpers for saturating arithmetic > tcg: Add generic vector helpers with a scalar operand > tcg/optimize: Handle vector opcodes during optimize > target/arm: Align vector registers > target/arm: Use vector infrastructure for aa64 add/sub/logic > target/arm: Use vector infrastructure for aa64 mov/not/neg > target/arm: Use vector infrastructure for aa64 dup/movi > target/arm: Use vector infrastructure for aa64 constant shifts > target/arm: Use vector infrastructure for aa64 compares > target/arm: Use vector infrastructure for aa64 multiplies > target/arm: Use vector infrastructure for aa64 orr/bic immediate > tcg/i386: Add vector operations > tcg/aarch64: Add vector operations > > Makefile.target | 4 +- > accel/tcg/tcg-runtime.h | 118 +++ > target/arm/cpu.h | 2 +- > tcg/aarch64/tcg-target.h | 25 +- > tcg/aarch64/tcg-target.opc.h | 3 + > tcg/i386/tcg-target.h | 41 +- > tcg/i386/tcg-target.opc.h | 13 + > tcg/tcg-gvec-desc.h | 49 + > tcg/tcg-op-gvec.h | 306 ++++++ > tcg/tcg-op.h | 52 +- > tcg/tcg-opc.h | 46 + > tcg/tcg.h | 87 ++ > accel/tcg/tcg-runtime-gvec.c | 997 +++++++++++++++++++ > target/arm/translate-a64.c | 979 ++++++++++++++----- > tcg/aarch64/tcg-target.inc.c | 588 ++++++++++- > tcg/i386/tcg-target.inc.c | 987 ++++++++++++++++++- > tcg/optimize.c | 150 +-- > tcg/tcg-op-gvec.c | 2215 ++++++++++++++++++++++++++++++++++++++++++ > tcg/tcg-op-vec.c | 389 ++++++++ > tcg/tcg-op.c | 42 +- > tcg/tcg-pool.inc.c | 115 ++- > tcg/tcg.c | 125 ++- > accel/tcg/Makefile.objs | 2 +- > configure | 48 + > tcg/README | 86 ++ > 25 files changed, 6973 insertions(+), 496 deletions(-) > create mode 100644 tcg/aarch64/tcg-target.opc.h > create mode 100644 tcg/i386/tcg-target.opc.h > create mode 100644 tcg/tcg-gvec-desc.h > create mode 100644 tcg/tcg-op-gvec.h > create mode 100644 accel/tcg/tcg-runtime-gvec.c > create mode 100644 tcg/tcg-op-gvec.c > create mode 100644 tcg/tcg-op-vec.c -- Alex Bennée
On 02/06/2018 08:24 AM, Alex Bennée wrote: > > I think this series is good to go. The x86 patch still require a volunteer to review ;)
Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > On 02/06/2018 08:24 AM, Alex Bennée wrote: >> >> I think this series is good to go. > > The x86 patch still require a volunteer to review ;) It looks good enough for me although I'm only a casual observer of x86 ;-) Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
Richard Henderson <richard.henderson@linaro.org> writes: > Changes since v11: > * Use dup_const more. > * Cleanup some gvec 2i and 2s routines. > * Use more helpers and less gotos in target/arm/translate-a64.c. I just noticed the aarch64 cross build breaks: n file included from /root/src/github.com/stsquad/qemu/tcg/tcg.c:296:0: /root/src/github.com/stsquad/qemu/tcg/aarch64/tcg-target.inc.c: In function 'tcg_out_dupi_vec': /root/src/github.com/stsquad/qemu/tcg/aarch64/tcg-target.inc.c:806:9: error: implicit declaration of function 'new_pool_l2' [-Werror=implicit-function-declaration] new_pool_l2(s, R_AARCH64_CONDBR19, s->code_ptr, 0, v64, v64); > > Changes since v10: > * Squashed a fixup patch which escaped my attention while preparing > the patch set. Ho hum. > > Changes since v9: > * Detect whether __attribute__((vector_size(16))) operations are > supported by the host compiler. This includes the case affecting > ppc64 where gcc-4.8.5 crashes. Note that gcc-7.2 does pass the > test on ppc64. > > * Dropped support for vector interleaves and element size changes. > My target/arm patches were failing RISU checks on a big-endian host. > I need to re-think what to do about host endianness and target > representation of vector operations crossing lanes. For now, only > support generic vector operations that are agnostic to element order. > > > r~ > > > Richard Henderson (20): > tcg: Allow multiple word entries into the constant pool > tcg: Add types and basic operations for host vectors > tcg: Standardize integral arguments to expanders > tcg: Add generic vector expanders > tcg: Add generic vector ops for constant shifts > tcg: Add generic vector ops for comparisons > tcg: Add generic vector ops for multiplication > tcg: Add generic helpers for saturating arithmetic > tcg: Add generic vector helpers with a scalar operand > tcg/optimize: Handle vector opcodes during optimize > target/arm: Align vector registers > target/arm: Use vector infrastructure for aa64 add/sub/logic > target/arm: Use vector infrastructure for aa64 mov/not/neg > target/arm: Use vector infrastructure for aa64 dup/movi > target/arm: Use vector infrastructure for aa64 constant shifts > target/arm: Use vector infrastructure for aa64 compares > target/arm: Use vector infrastructure for aa64 multiplies > target/arm: Use vector infrastructure for aa64 orr/bic immediate > tcg/i386: Add vector operations > tcg/aarch64: Add vector operations > > Makefile.target | 4 +- <snip> -- Alex Bennée
Alex Bennée <alex.bennee@linaro.org> writes: > Richard Henderson <richard.henderson@linaro.org> writes: > >> Changes since v11: >> * Use dup_const more. >> * Cleanup some gvec 2i and 2s routines. >> * Use more helpers and less gotos in target/arm/translate-a64.c. > > I just noticed the aarch64 cross build breaks: > > n file included from /root/src/github.com/stsquad/qemu/tcg/tcg.c:296:0: > /root/src/github.com/stsquad/qemu/tcg/aarch64/tcg-target.inc.c: In function 'tcg_out_dupi_vec': > /root/src/github.com/stsquad/qemu/tcg/aarch64/tcg-target.inc.c:806:9: error: implicit declaration of function 'new_pool_l2' [-Werror=implicit-function-declaration] > new_pool_l2(s, R_AARCH64_CONDBR19, s->code_ptr, 0, v64, v64); Ignore me, bad patch application that only affected that build. -- Alex Bennée