Message ID | 1597765847-16637-1-git-send-email-tsimpson@quicinc.com (mailing list archive) |
---|---|
Headers | show |
Series | Hexagon patch series | expand |
Patchew URL: https://patchew.org/QEMU/1597765847-16637-1-git-send-email-tsimpson@quicinc.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 1597765847-16637-1-git-send-email-tsimpson@quicinc.com Subject: [RFC PATCH v3 00/34] Hexagon patch series === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/1597765847-16637-1-git-send-email-tsimpson@quicinc.com -> patchew/1597765847-16637-1-git-send-email-tsimpson@quicinc.com - [tag update] patchew/20200610120426.12826-1-vsementsov@virtuozzo.com -> patchew/20200610120426.12826-1-vsementsov@virtuozzo.com Switched to a new branch 'test' 149d5a8 Hexagon build infrastructure 4db60b1 Hexagon (tests/tcg/hexagon) TCG tests 3799611 Hexagon (linux-user/hexagon) Linux user emulation a71a593 Hexagon (target/hexagon) translation edab971 Hexagon (target/hexagon) TCG for instructions with multiple definitions e932b7e Hexagon (target/hexagon) TCG generation 77ada1d Hexagon (target/hexagon) TCG generation helpers be70802 Hexagon (target/hexagon) instruction classes 33ce822 Hexagon (target/hexagon) macros referenced in instruction semantics f44d484 Hexagon (target/hexagon) macros to interface with the generator c4e4b84 Hexagon (target/hexagon) opcode data structures 64f25a3 Hexagon (target/hexagon) generater phase 4 - decode tree 799a8cb Hexagon (target/hexagon) generator phase 3 - C preprocessor for decode tree fd0b8ef Hexagon (target/hexagon) generator phase 2 - generate header files 01ee39f Hexagon (target/hexagon) generator phase 1 - C preprocessor for semantics 56a8168 Hexagon (target/hexagon/imported) arch import - instruction encoding fe828ec Hexagon (target/hexagon/imported) arch import - instruction semantics 47fcbc9 Hexagon (target/hexagon/imported) arch import - macro definitions 032fa2d Hexagon (target/hexagon) utility functions 90e2392 Hexagon (target/hexagon) instruction printing e4a75c2 Hexagon (target/hexagon) instruction/packet decode 720bbdf Hexagon (target/hexagon) register map 4a5c3d8 Hexagon (target/hexagon) instruction attributes 01bf963 Hexagon (target/hexagon) register fields 934c416 Hexagon (target/hexagon) instruction and packet types e905475 Hexagon (target/hexagon) architecture types e10c00c Hexagon (target/hexagon) GDB Stub 54222f4 Hexagon (target/hexagon) scalar core helpers 8c90070 Hexagon (disas) disassembler fc17f04 Hexagon (target/hexagon) register names 87807c4 Hexagon (target/hexagon) scalar core definition 28282ee Hexagon (include/elf.h) ELF machine definition acf59c5 Hexagon (target/hexagon) README 360594d Hexagon Update MAINTAINERS file === OUTPUT BEGIN === 1/34 Checking commit 360594de302c (Hexagon Update MAINTAINERS file) 2/34 Checking commit acf59c5f8d57 (Hexagon (target/hexagon) README) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #13: new file mode 100644 total: 0 errors, 1 warnings, 254 lines checked Patch 2/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 3/34 Checking commit 28282eeb25c8 (Hexagon (include/elf.h) ELF machine definition) 4/34 Checking commit 87807c4ee5ed (Hexagon (target/hexagon) scalar core definition) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #13: new file mode 100644 total: 0 errors, 1 warnings, 580 lines checked Patch 4/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 5/34 Checking commit fc17f04e763d (Hexagon (target/hexagon) register names) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #11: new file mode 100644 total: 0 errors, 1 warnings, 83 lines checked Patch 5/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 6/34 Checking commit 8c900700b508 (Hexagon (disas) disassembler) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #26: new file mode 100644 total: 0 errors, 1 warnings, 76 lines checked Patch 6/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 7/34 Checking commit 54222f424b69 (Hexagon (target/hexagon) scalar core helpers) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #14: new file mode 100644 total: 0 errors, 1 warnings, 401 lines checked Patch 7/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 8/34 Checking commit e10c00cef005 (Hexagon (target/hexagon) GDB Stub) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #26: new file mode 100644 total: 0 errors, 1 warnings, 63 lines checked Patch 8/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 9/34 Checking commit e9054759731f (Hexagon (target/hexagon) architecture types) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #13: new file mode 100644 total: 0 errors, 1 warnings, 43 lines checked Patch 9/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 10/34 Checking commit 934c416b066b (Hexagon (target/hexagon) instruction and packet types) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #14: new file mode 100644 total: 0 errors, 1 warnings, 75 lines checked Patch 10/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 11/34 Checking commit 01bf963045a8 (Hexagon (target/hexagon) register fields) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #13: new file mode 100644 ERROR: Macros with complex values should be enclosed in parenthesis #84: FILE: target/hexagon/reg_fields.h:33: +#define DEF_REG_FIELD(TAG, NAME, START, WIDTH, DESCRIPTION) \ + TAG, total: 1 errors, 1 warnings, 146 lines checked Patch 11/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 12/34 Checking commit 4a5c3d8b0344 (Hexagon (target/hexagon) instruction attributes) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #11: new file mode 100644 ERROR: Macros with complex values should be enclosed in parenthesis #37: FILE: target/hexagon/attribs.h:22: +#define DEF_ATTRIB(NAME, ...) A_##NAME, total: 1 errors, 1 warnings, 130 lines checked Patch 12/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 13/34 Checking commit 720bbdf3c5d8 (Hexagon (target/hexagon) register map) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #16: new file mode 100644 total: 0 errors, 1 warnings, 38 lines checked Patch 13/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 14/34 Checking commit e4a75c280804 (Hexagon (target/hexagon) instruction/packet decode) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #22: new file mode 100644 ERROR: Macros with complex values should be enclosed in parenthesis #118: FILE: target/hexagon/decode.c:92: +#define DECODE_SEPARATOR_BITS(START, WIDTH) NULL, START, WIDTH ERROR: Macros with multiple statements should be enclosed in a do - while loop #721: FILE: target/hexagon/q6v_decode.c:51: +#define DECODE_OPINFO(TAG, BEH) \ + case TAG: \ + { BEH } \ + break; \ + total: 2 errors, 1 warnings, 1005 lines checked Patch 14/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 15/34 Checking commit 90e239256680 (Hexagon (target/hexagon) instruction printing) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #11: new file mode 100644 ERROR: Macros with multiple statements should be enclosed in a do - while loop #61: FILE: target/hexagon/printinsn.c:46: +#define DEF_PRINTINFO(TAG, FMT, ...) \ + case TAG: \ + snprintf(buf, n, FMT, __VA_ARGS__);\ + break; total: 1 errors, 1 warnings, 120 lines checked Patch 15/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 16/34 Checking commit 032fa2d6f2f2 (Hexagon (target/hexagon) utility functions) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #13: new file mode 100644 WARNING: Block comments use a leading /* on a separate line #1326: FILE: target/hexagon/fma_emu.c:470: + /* result zero */ \ WARNING: Block comments use a leading /* on a separate line #1334: FILE: target/hexagon/fma_emu.c:478: + /* Normalize right */ \ WARNING: Block comments use a leading /* on a separate line #1335: FILE: target/hexagon/fma_emu.c:479: + /* We want MANTBITS bits of mantissa plus the leading one. */ \ WARNING: Block comments use a leading /* on a separate line #1336: FILE: target/hexagon/fma_emu.c:480: + /* That means that we want MANTBITS+1 bits, or 0x000000000000FF_FFFF */ \ WARNING: Block comments use a leading /* on a separate line #1337: FILE: target/hexagon/fma_emu.c:481: + /* So we need to normalize right while the high word is non-zero and \ WARNING: Block comments should align the * on each line #1338: FILE: target/hexagon/fma_emu.c:482: + /* So we need to normalize right while the high word is non-zero and \ + * while the low word is nonzero when masked with 0xffe0_0000_0000_0000 */ \ WARNING: Block comments use a leading /* on a separate line #1342: FILE: target/hexagon/fma_emu.c:486: + /* \ WARNING: Block comments use a leading /* on a separate line #1352: FILE: target/hexagon/fma_emu.c:496: + /* \ WARNING: Block comments use a leading /* on a separate line #1359: FILE: target/hexagon/fma_emu.c:503: + /* \ WARNING: Block comments use a leading /* on a separate line #1368: FILE: target/hexagon/fma_emu.c:512: + /* OK, we're relatively canonical... now we need to round */ \ WARNING: Block comments use a leading /* on a separate line #1373: FILE: target/hexagon/fma_emu.c:517: + /* Chop and we're done */ \ WARNING: Block comments use a leading /* on a separate line #1387: FILE: target/hexagon/fma_emu.c:531: + /* round up if guard is 1, down if guard is zero */ \ WARNING: Block comments use a leading /* on a separate line #1390: FILE: target/hexagon/fma_emu.c:534: + /* exactly .5, round up if odd */ \ WARNING: Block comments use a leading /* on a separate line #1396: FILE: target/hexagon/fma_emu.c:540: + /* \ WARNING: Block comments use a leading /* on a separate line #1405: FILE: target/hexagon/fma_emu.c:549: + /* Overflow? */ \ WARNING: Block comments use a leading /* on a separate line #1407: FILE: target/hexagon/fma_emu.c:551: + /* Yep, inf result */ \ WARNING: Block comments use a leading /* on a separate line #1429: FILE: target/hexagon/fma_emu.c:573: + /* Underflow? */ \ WARNING: Block comments use a leading /* on a separate line #1431: FILE: target/hexagon/fma_emu.c:575: + /* Leading one means: No, we're normal. So, we should be done... */ \ total: 0 errors, 19 warnings, 1623 lines checked Patch 16/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 17/34 Checking commit 47fcbc98752d (Hexagon (target/hexagon/imported) arch import - macro definitions) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #15: new file mode 100755 total: 0 errors, 1 warnings, 1529 lines checked Patch 17/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 18/34 Checking commit fe828ec04435 (Hexagon (target/hexagon/imported) arch import - instruction semantics) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #19: new file mode 100644 total: 0 errors, 1 warnings, 5337 lines checked Patch 18/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 19/34 Checking commit 56a816808b60 (Hexagon (target/hexagon/imported) arch import - instruction encoding) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #14: new file mode 100644 total: 0 errors, 1 warnings, 2385 lines checked Patch 19/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 20/34 Checking commit 01ee39f0b141 (Hexagon (target/hexagon) generator phase 1 - C preprocessor for semantics) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #17: new file mode 100644 ERROR: suspicious ; after while (0) #82: FILE: target/hexagon/gen_semantics.c:61: + } while (0); total: 1 errors, 1 warnings, 88 lines checked Patch 20/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 21/34 Checking commit fd0b8efdcba8 (Hexagon (target/hexagon) generator phase 2 - generate header files) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #30: new file mode 100755 total: 0 errors, 1 warnings, 1354 lines checked Patch 21/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 22/34 Checking commit 799a8cb9426a (Hexagon (target/hexagon) generator phase 3 - C preprocessor for decode tree) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #15: new file mode 100644 ERROR: Macros with complex values should be enclosed in parenthesis #76: FILE: target/hexagon/gen_dectree_import.c:57: +#define REGINFO(TAG, REGINFO, RREGS, WREGS) RREGS, ERROR: Macros with complex values should be enclosed in parenthesis #85: FILE: target/hexagon/gen_dectree_import.c:66: +#define REGINFO(TAG, REGINFO, RREGS, WREGS) WREGS, ERROR: suspicious ; after while (0) #182: FILE: target/hexagon/gen_dectree_import.c:163: + } while (0); total: 3 errors, 1 warnings, 191 lines checked Patch 22/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 23/34 Checking commit 64f25a3e9492 (Hexagon (target/hexagon) generater phase 4 - decode tree) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #13: new file mode 100755 total: 0 errors, 1 warnings, 352 lines checked Patch 23/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 24/34 Checking commit c4e4b843d932 (Hexagon (target/hexagon) opcode data structures) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #11: new file mode 100644 ERROR: Macros with complex values should be enclosed in parenthesis #56: FILE: target/hexagon/opcodes.c:41: +#define REGINFO(TAG, REGINFO, RREGS, WREGS) REGINFO, ERROR: Macros with complex values should be enclosed in parenthesis #66: FILE: target/hexagon/opcodes.c:51: +#define REGINFO(TAG, REGINFO, RREGS, WREGS) RREGS, ERROR: Macros with complex values should be enclosed in parenthesis #76: FILE: target/hexagon/opcodes.c:61: +#define REGINFO(TAG, REGINFO, RREGS, WREGS) WREGS, ERROR: Macros with complex values should be enclosed in parenthesis #180: FILE: target/hexagon/opcodes.c:165: +#define ATTRIBS(...) , ## __VA_ARGS__, 0 total: 4 errors, 1 warnings, 277 lines checked Patch 24/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 25/34 Checking commit f44d4844c738 (Hexagon (target/hexagon) macros to interface with the generator) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #13: new file mode 100644 total: 0 errors, 1 warnings, 364 lines checked Patch 25/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 26/34 Checking commit 33ce822b03e9 (Hexagon (target/hexagon) macros referenced in instruction semantics) 27/34 Checking commit be70802f716c (Hexagon (target/hexagon) instruction classes) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #13: new file mode 100644 ERROR: Macros with complex values should be enclosed in parenthesis #138: FILE: target/hexagon/iclass.h:27: +#define DEF_PP_ICLASS32(TYPE, SLOTS, UNITS) ICLASS_FROM_TYPE(TYPE), ERROR: Macros with complex values should be enclosed in parenthesis #144: FILE: target/hexagon/iclass.h:33: +#define DEF_EE_ICLASS32(TYPE, SLOTS, UNITS) ICLASS_FROM_TYPE(TYPE), total: 2 errors, 1 warnings, 186 lines checked Patch 27/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 28/34 Checking commit 77ada1dc6124 (Hexagon (target/hexagon) TCG generation helpers) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #14: new file mode 100644 total: 0 errors, 1 warnings, 268 lines checked Patch 28/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 29/34 Checking commit e932b7e1745b (Hexagon (target/hexagon) TCG generation) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #13: new file mode 100644 total: 0 errors, 1 warnings, 80 lines checked Patch 29/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 30/34 Checking commit edab9714dc4f (Hexagon (target/hexagon) TCG for instructions with multiple definitions) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #14: new file mode 100644 total: 0 errors, 1 warnings, 213 lines checked Patch 30/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 31/34 Checking commit a71a593d99c6 (Hexagon (target/hexagon) translation) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #17: new file mode 100644 total: 0 errors, 1 warnings, 833 lines checked Patch 31/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 32/34 Checking commit 3799611b203f (Hexagon (linux-user/hexagon) Linux user emulation) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #41: new file mode 100644 total: 0 errors, 1 warnings, 1056 lines checked Patch 32/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 33/34 Checking commit 4db60b131aa7 (Hexagon (tests/tcg/hexagon) TCG tests) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #37: new file mode 100644 total: 0 errors, 1 warnings, 2728 lines checked Patch 33/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 34/34 Checking commit 149d5a836f53 (Hexagon build infrastructure) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #47: new file mode 100644 WARNING: line over 80 characters #70: FILE: scripts/qemu-binfmt-conf.sh:139: +hexagon_magic='\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\xa4\x00' ERROR: line over 90 characters #71: FILE: scripts/qemu-binfmt-conf.sh:140: +hexagon_mask='\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff' total: 1 errors, 2 warnings, 243 lines checked Patch 34/34 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/1597765847-16637-1-git-send-email-tsimpson@quicinc.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On 8/18/20 8:50 AM, Taylor Simpson wrote: > This series adds support for the Hexagon processor with Linux user support > > See patch 02/34 Hexagon README for detailed information. > > Once the series is applied, the Hexagon port will pass "make check-tcg". > The series also includes Hexagon-specific tests in tcg/tests/hexagon. > > We have a parallel effort to make the Hexagon Linux toolchain inside a docker > container publically available. Oh, excellent. > *** Future items under consideration *** > Use qemu softfloat This is a blocker. It's definitely not hard. > Use qemu decodetree This would certainly clean up all of the string processing that I mentioned. It seems like it would be just as easy to convert into the decodetree format as what you're currently doing for dectree_generated.h. Indeed, easier, since you only need the ones that are TERMINAL(). All of the other things labeled TABLE_LINK are handled by decodetree itself. Anyway, I've completed what review I planed to do against this version. r~
Richard, Thank you so much for the feedback. I really appreciate it. I'll get to work addressing the issues. Since some of the items will take longer than others, please advise whether it's preferred to send intermediate updates or wait until they are all addressed. Taylor > -----Original Message----- > From: Richard Henderson <richard.henderson@linaro.org> > Sent: Friday, August 28, 2020 9:27 PM > To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org > Cc: philmd@redhat.com; laurent@vivier.eu; riku.voipio@iki.fi; > aleksandar.m.mail@gmail.com; ale@rev.ng > Subject: Re: [RFC PATCH v3 00/34] Hexagon patch series > > On 8/18/20 8:50 AM, Taylor Simpson wrote: > > This series adds support for the Hexagon processor with Linux user support > > > > See patch 02/34 Hexagon README for detailed information. > > > > Once the series is applied, the Hexagon port will pass "make check-tcg". > > The series also includes Hexagon-specific tests in tcg/tests/hexagon. > > > > We have a parallel effort to make the Hexagon Linux toolchain inside a > docker > > container publically available. > > Oh, excellent. > > > *** Future items under consideration *** > > Use qemu softfloat > > This is a blocker. It's definitely not hard. > > > Use qemu decodetree > > This would certainly clean up all of the string processing that I mentioned. > It seems like it would be just as easy to convert into the decodetree format > as > what you're currently doing for dectree_generated.h. Indeed, easier, since > you > only need the ones that are TERMINAL(). All of the other things labeled > TABLE_LINK are handled by decodetree itself. > > Anyway, I've completed what review I planed to do against this version. > > > r~
On 8/30/20 1:47 PM, Taylor Simpson wrote: > Richard, > > Thank you so much for the feedback. I really appreciate it. > > I'll get to work addressing the issues. Since some of the items will take longer than others, please advise whether it's preferred to send intermediate updates or wait until they are all addressed. I don't mind intermediate updates. Just keep a list in the cover letter of the things that are still on the to-do list, and I'll not focus on those. We could also talk about what portions of the to-do list are blocker, and what can be done via normal development. Because neither you nor I want to carry around this huge patch set forever. r~
> -----Original Message----- > From: Richard Henderson <richard.henderson@linaro.org> > Sent: Sunday, August 30, 2020 5:33 PM > To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org > Cc: philmd@redhat.com; laurent@vivier.eu; riku.voipio@iki.fi; > aleksandar.m.mail@gmail.com; ale@rev.ng > Subject: Re: [RFC PATCH v3 00/34] Hexagon patch series > > I don't mind intermediate updates. Just keep a list in the cover letter of the > things that are still on the to-do list, and I'll not focus on those. > > We could also talk about what portions of the to-do list are blocker, and what > can be done via normal development. Because neither you nor I want to > carry > around this huge patch set forever. OK, here's the list of items. Let me know if I missed anything. I'll indicate which ones can be done quickly and which ones would take more time. I added a column for blocker if you or anyone else has input on that. PatchItemEffortBlocker Use qemu softfloat??Yes Use qemu decodetree.py?? SeveralUse const when appropriatesmall SeveralRemove anything after g_assert_not_reachedsmall SeveralFix log_store32/64 add/remove/add in patch seriessmall SeveralFollow naming guidelines for structs and enumssmall 04Move decls to cpu-param.hsmall 04Remove CONFIG_USER_ONLY ifdef'ssmall 04Remove DEBUG_HEXAGONsmall 04Remove stack pointer modification hack, use qemu mechanismsmall 04Add property x-lldb-compat to control output in logsmall 06Include instruction and raw bytes in disassemblysmall 07Use DEF_HELPER_FLAGSsmall 07, 26Endianness of merge_bytessmall 07Fix overlap testsmall 07Remove HELPER(debug_value)/HELPER(debug_value_i64)small 09Include "qemu/osdep.h" instead of <stdint.h>small 10 (and others)Stick with stdint.h types except in imported filessmall 11Remove description from reg field definitionssmall 13Move regmap.h into decode.csmall 14, 27Use bit mask instead of strings in decodingsmall 14Add comments to decodersmall 16Use qemu/int128.hmedium 17Squash patches dealing with imported filessmall 24Use qemu/bitops.h for instruction attributessmall 24Fix initialization of opcode_short_semanticssmall 24Change if (p == NULL) { g_assert_not_reached(); } to assert(p != NULL)small 25Expand DECL/READ/WRITE/FREE macros into generated codesmall 26Rewrite fINSERT*, fEXTRACT*, f?XTN macrossmall 26Investigate fRND macrosmall 26Change REG = REG to (VOID)REG to suppress compiler warningsmall 27Remove multiple includes of imported/iclass.defsmall 28Move genptr_helpers.h into genptr.csmall 28Remove unneeded tempssmall 28Use tcg_gen_deposit_tl and tcg_gen_extract_tl when dealing with p3_0small 29Size opcode_genptr[] properly and initialize with [TAG] = generate_##TAGsmall 30Don't generate helpers for instructions that are overriddensmall Don't include "gen_tcg.h" in helper.h 31Use bitmask for ctx->reg_log instead of an arraysmall 31Use tcg_gen_extract_i32 for gen_slot_cancelled_checksmall 31Properly deal with reading instructions across a page boundary and toomedium many instructions before finding end-of-packet 31Don't set PC at the beginning of every packetmedium 31Don't set slot_cancelled unless neededsmall 31Don't set hex_pred_written unless neededmedium 31Change cancelled variable to not localsmall 31Remove unnecessary tempsmall 31Let tcg_gen_addi_tl check for zerosmall 31Move gen_exec_counters to end of TB instead of every packetmedium 31Move end of TB handling to hexagon_tr_tb_stopsmall
On 8/31/20 10:57 AM, Taylor Simpson wrote: > OK, here's the list of items. Let me know if I missed anything. I'll > indicate which ones can be done quickly and which ones would take more time. > I added a column for blocker if you or anyone else has input on that. > > PatchItemEffortBlocker > Use qemu softfloat??Yes Hmm, this table didn't render. Below, yes/no for blocker column. > Use qemu decodetree.py?? no > SeveralUse const when appropriatesmall yes > SeveralRemove anything after g_assert_not_reachedsmall yes > SeveralFix log_store32/64 add/remove/add in patch seriessmall yes > SeveralFollow naming guidelines for structs and enumssmall yes > 04Move decls to cpu-param.hsmall Yes. The only reason this even compiled is that you don't do system mode yet. ;-) > 04Remove CONFIG_USER_ONLY ifdef'ssmall yes > 04Remove DEBUG_HEXAGONsmall yes > 04Remove stack pointer modification hack, use qemu mechanismsmall yes > 04Add property x-lldb-compat to control output in logsmall yes > 06Include instruction and raw bytes in disassemblysmall yes > 07Use DEF_HELPER_FLAGSsmall no, but should be easy. > 07, 26Endianness of merge_bytessmall Yes, one way or another; hopefully by removing all of the merge_bytes and using probe_read. > 07Fix overlap testsmall yes > 07Remove HELPER(debug_value)/HELPER(debug_value_i64)small yes > 09Include "qemu/osdep.h" instead of <stdint.h>small yes > 10 (and others)Stick with stdint.h types except in imported filessmall yes > 11Remove description from reg field definitionssmall yes > 13Move regmap.h into decode.csmall yes > 14, 27Use bit mask instead of strings in decodingsmall no, but should be easy. > 14Add comments to decodersmall yes > 16Use qemu/int128.hmedium no > 17Squash patches dealing with imported filessmall yes > 24Use qemu/bitops.h for instruction attributessmall no > 24Fix initialization of opcode_short_semanticssmall yes > 24Change if (p == NULL) { g_assert_not_reached(); } to assert(p != NULL)small no > 25Expand DECL/READ/WRITE/FREE macros into generated codesmall Yes. In the end I think some of these will in the end want to be helper functions. As I was thinking how to best write A2_add, I was thinking /* TODO: You currently have an "offset" parameter to DECL_REG. I can't see that it is ever used? I would *hope* that this could be resolved earlier, so that by this time insn->regno[*] is absolute. */ static int resolve_regno(Insn *insn, int slot, int off); /* Return hex_new_value[regno]; log the write. */ static TCGv reg_for_write(DisasContext *ctx, int regno); /* Return hex_reg[regno]; force up-to-date value for PC. */ static TCGv reg_for_read(DisasContext *ctx, int regno); /* if (preg) hex_new_value[regno] = hex_reg[regno], or !preg if !test_positive. Leaves hex_new_value[] canonical for gen_reg_writes, no extra temporary required. */ static void gen_cancel_reg(DisasContext *ctx, int preg, int rreg, bool test_positive); DEF_TCG_FUNC(A2_add) { int rd = resolve_regno(insn, 0, 0); int rs = resolve_regno(insn, 1, 0); int rt = resolve_regno(insn, 2, 0); tcg_gen_add_tl(reg_for_write(ctx, rd), reg_for_read(ctx, rs), reg_for_read(ctx, rt)); } DEF_TCG_FUNC(A2_paddit) { int pu = resolve_regno(insn, 0, 0); int rd = resolve_regno(insn, 1, 0); int rs = resolve_regno(insn, 2, 0); int rt = resolve_regno(insn, 3, 0); tcg_gen_add_tl(reg_for_write(ctx, rd), reg_for_read(ctx, rs), reg_for_read(ctx, rt)); gen_cancel_reg(ctx, insn, rd, pu, true); } However, I don't think we have to have a comprehensive set of these now. We can expand everything into the generator to start, then adjust the generator as we add helper functions and use them within the overrides. > 26Rewrite fINSERT*, fEXTRACT*, f?XTN macrossmall yes > 26Investigate fRND macrosmall no, but should be easy. > 26Change REG = REG to (VOID)REG to suppress compiler warningsmall yes > 27Remove multiple includes of imported/iclass.defsmall yes > 28Move genptr_helpers.h into genptr.csmall yes > 28Remove unneeded tempssmall no > 28Use tcg_gen_deposit_tl and tcg_gen_extract_tl when dealing with p3_0small no > 29Size opcode_genptr[] properly and initialize with [TAG] = generate_##TAGsmall yes; i think this will fall out of other changes. > 30Don't generate helpers for instructions that are overriddensmall yes > Don't include "gen_tcg.h" in helper.h yes > 31Use bitmask for ctx->reg_log instead of an arraysmall yes > 31Use tcg_gen_extract_i32 for gen_slot_cancelled_checksmall yes > 31Properly deal with reading instructions across a page boundary and toomedium > many instructions before finding end-of-packet Yes, this should be easy. Unless there's some surprise in the code I have already suggested code. > 31Don't set PC at the beginning of every packetmedium no > 31Don't set slot_cancelled unless neededsmall no > 31Don't set hex_pred_written unless neededmedium no > 31Change cancelled variable to not localsmall yes > 31Remove unnecessary tempsmall yes > 31Let tcg_gen_addi_tl check for zerosmall yes > 31Move gen_exec_counters to end of TB instead of every packetmedium no > 31Move end of TB handling to hexagon_tr_tb_stopsmall yes r~
> -----Original Message----- > From: Richard Henderson <richard.henderson@linaro.org> > Sent: Monday, August 31, 2020 2:44 PM > To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org > Cc: philmd@redhat.com; laurent@vivier.eu; riku.voipio@iki.fi; > aleksandar.m.mail@gmail.com; ale@rev.ng > Subject: Re: [RFC PATCH v3 00/34] Hexagon patch series > > On 8/31/20 10:57 AM, Taylor Simpson wrote: > > OK, here's the list of items. Let me know if I missed anything. I'll > > indicate which ones can be done quickly and which ones would take more > time. > > I added a column for blocker if you or anyone else has input on that. > > > > PatchItemEffortBlocker > > Use qemu softfloat??Yes > > Hmm, this table didn't render. Below, yes/no for blocker column. Sorry about that - not sure what happened. I will work all those you marked "yes" or "no, but should be easy". > > 25Expand DECL/READ/WRITE/FREE macros into generated codesmall > > Yes. > > In the end I think some of these will in the end want to be helper functions. > As I was thinking how to best write A2_add, I was thinking See my response to the thread on patch 30/34. Since you mention A2_paddit, here's what it would look like assuming it is overridden. static void generate_A2_paddt(CPUHexagonState *env, DisasContext *ctx, insn_t *insn, packet_t *pkt) { /* A2_paddit */ int PuN = insn->regno[0]; TCGv PuV = hex_pred[PuN]; Int RdN = insn->regno[1]; TCGv RdV = tcg_temp_local_new(); if (!is_preloaded(ctx, RdN)) { tcg_gen_mov_tl(hex_new_value[RdN], hex_gpr[RdN]); } int RsN = insn->regno[2]; TCGv RsV = hex_gpr[RsN]; int siV = insn->immed[0]; fGEN_TCG_A2_paddit({if(fLSBOLD(PuV)){fIMMEXT(siV); RdV=RsV+siV;} else {CANCEL;}}); gen_log_reg_write(RdN, RdV, insn->slot, 1); /* Only does the write if we haven't cancelled */ ctx_log_reg_write(ctx, RdN); tcg_temp_free(RdV); /* A2_paddit */ } Here's what the override looks like (there are a bunch of these, so we have a helper macro which could also be a function) /* Predicated add instructions */ #define GEN_TCG_padd(PRED, ADD) \ do { \ TCGv LSB = tcg_temp_new(); \ TCGv mask = tcg_temp_new(); \ TCGv zero = tcg_const_tl(0); \ PRED; \ ADD; \ tcg_gen_movi_tl(mask, 1 << insn->slot); \ tcg_gen_or_tl(mask, hex_slot_cancelled, mask); \ tcg_gen_movcond_tl(TCG_COND_NE, hex_slot_cancelled, LSB, zero, \ hex_slot_cancelled, mask); \ tcg_temp_free(LSB); \ tcg_temp_free(mask); \ tcg_temp_free(zero); \ } while (0) #define fGEN_TCG_A2_paddit(SHORTCODE) \ GEN_TCG_padd(fLSBOLD(PuV), tcg_gen_addi_tl(RdV, RsV, siV))
On 8/30/20 3:47 PM, Taylor Simpson wrote: > Richard, > > Thank you so much for the feedback. I really appreciate it. > > I'll get to work addressing the issues. Since some of the items will take longer than others, please advise whether it's preferred to send intermediate updates or wait until they are all addressed. > > Taylor Which branch of https://github.com/quic/qemu do I pull to try this without scraping 30 patches out of the mailing list? >>> Once the series is applied, the Hexagon port will pass "make check-tcg". >>> The series also includes Hexagon-specific tests in tcg/tests/hexagon. >>> >>> We have a parallel effort to make the Hexagon Linux toolchain inside a >> docker >>> container publically available. >> >> Oh, excellent. Is that a follow-up to https://www.spinics.net/lists/linux-hexagon/msg01706.html and is this a clang toolchain or a gcc toolchain? I've noticed gcc 9.2 has hexagon in config.guess and config.sub, but the only other file outside of the test suite containing the word "hexagon" in a case insensitive match is the Changelog saying Linas Veptas added it to config.guess and config.sub in 2011. And while https://github.com/quic has a musl fork it doesn't have any compiler or linker forks... Rob
> -----Original Message----- > From: Qemu-devel <qemu-devel-bounces+bcain=quicinc.com@nongnu.org> > On Behalf Of Rob Landley ... > > On 8/30/20 3:47 PM, Taylor Simpson wrote: > > Richard, > > > > Thank you so much for the feedback. I really appreciate it. > > > > I'll get to work addressing the issues. Since some of the items will take longer > than others, please advise whether it's preferred to send intermediate updates > or wait until they are all addressed. > > > > Taylor > > Which branch of https://github.com/quic/qemu do I pull to try this without > scraping 30 patches out of the mailing list? IIRC this patch series was "small_series_v3" > >>> Once the series is applied, the Hexagon port will pass "make check-tcg". > >>> The series also includes Hexagon-specific tests in tcg/tests/hexagon. > >>> > >>> We have a parallel effort to make the Hexagon Linux toolchain inside > >>> a > >> docker > >>> container publically available. > >> > >> Oh, excellent. > > Is that a follow-up to https://www.spinics.net/lists/linux- > hexagon/msg01706.html > and is this a clang toolchain or a gcc toolchain? It's a clang toolchain. > I've noticed gcc 9.2 has hexagon in config.guess and config.sub, but the only > other file outside of the test suite containing the word "hexagon" in a case > insensitive match is the Changelog saying Linas Veptas added it to config.guess > and config.sub in 2011. And while https://github.com/quic has a musl fork it > doesn't have any compiler or linker forks... clang and lld support for hexagon exists in the upstream llvm-project repo. -Brian