By the time the initial binutils support was implemented, we had no real handling for many of the branch specifiers. It was decided that we should postpone these for a while. The real support for Python assembly was implemented only in September 2022, and has not been merged into binutils since then yet. note, not to be confused with bug #1003, must avoid duplication.
Rebased svp64 branch. Surprisingly they introduced more changes than I expected, so it took a while. After thinking for a while, I decided that I'll go with the following approach: 1. Launch src/openpower/sv/trans/test_pysvp64dis.py and grab all instructions it produces. 2. Go over all instructions and fix specifiers which are missing, one by one. 3. Once it's done, we're on par with our home-grown assembly with specifiers. The only problem I see here is that our tests also check CR and branches, so it might sometimes be difficult to decouple these changes (other than by discarding anything but specifiers). 1. Is it correct that I should preferrably restrict the changes for this task with specifiers-related changes only? 2. What should I do if I cannot add a specifier (say some CR or branch specific one) unless I also introduce support for other bits? Some specifiers need updates for CR/branch modes, which are not yet implemented in binutils correctly (actually binutils code even predates CR/branches in the way they're implemented in our assembly).
OK, it took a couple of days, where mostly I had to spend time trying to recall how on Earth these things work. :-) I've added the code which allows binutils to recognize specifiers it previously lacked. https://git.libre-soc.org/?p=binutils-gdb.git;a=commitdiff;h=4500e1cec69831512095f4f4b36c3c841fb60f96;hp=f75d3c24e6e403032bf180561b2bd25d7857d8a5 Two other commits regenerated the headers and sources (caveat, huge diffs): https://git.libre-soc.org/?p=binutils-gdb.git;a=commitdiff;h=4e45cd03d3d5ea59cef577df03bc9461f512a883 https://git.libre-soc.org/?p=binutils-gdb.git;a=commitdiff;h=20aa766718bb65a15ea65104a90f477bdd8108c0 This needed a minor workaround on sv_binutils side (not a real fix, this chunk should be refactored, since I changed the way extras are handled in insndb long ago): https://git.libre-soc.org/?p=openpower-isa.git;a=blobdiff;f=src/openpower/sv/sv_binutils.py;h=c2ba879689147107eca223d6ec65da8269f6cca4;hp=b4dea0acf62ed1419fb36cda661c99f2b702d3ee;hb=e3ea73ea5f589bbba5063d62279c2230aad6c254;hpb=59f1579956f07efe2611f6892a923764a0778ab2 At this point, we don't yet check whether binutils generate the same assembly as python canonical code (almost no chances it does since the binutils are vastly outdated and do not even use auto-generated functions everywhere). However, I suppose the rest is to be done in https://bugs.libre-soc.org/show_bug.cgi?id=1003. Any objections, suggestions, ideas?
(In reply to Dmitry Selyutin from comment #2) > OK, it took a couple of days, where mostly I had to spend time trying to > recall how on Earth these things work. :-) > I've added the code which allows binutils to recognize specifiers it > previously lacked. nice. it's going to be an iterative process. > However, I suppose the rest is to be done in > https://bugs.libre-soc.org/show_bug.cgi?id=1003. Any objections, > suggestions, ideas? idea: make sure chacha20 works at least. konstantinos i'm happy to raise a new bugreport with budget for that?
Nice idea! Could you please point me to the place with the code to check? Beware: depending on how many instructions are used, it might overlap with 1003 even more: I expect binutils are rusty wrt assembly compared to our Python code.
(In reply to Dmitry Selyutin from comment #4) > Nice idea! Could you please point me to the place with the code to check? > Beware: depending on how many instructions are used, it might overlap with > 1003 even more: I expect binutils are rusty wrt assembly compared to our > Python code. openpower-isa, although the media/ mp3 tests will be easier, the crypto ones have a lot more dependencies.
Short (as possible) summary on media tests. 1. The current master branch is broken: Traceback (most recent call last): File "/usr/local/bin/pysvp64asm", line 11, in <module> load_entry_point('libresoc-openpower-isa', 'console_scripts', 'pysvp64asm')() File "/home/ghostmansd/src/openpower-isa/src/openpower/sv/trans/svp64.py", line 212, in asm_process lst = '; '.join(lst) File "/home/ghostmansd/src/openpower-isa/src/openpower/sv/trans/svp64.py", line 89, in translate_one insn = WordInstruction.assemble(record=record, arguments=fields) File "/home/ghostmansd/src/openpower-isa/src/openpower/decoder/power_insn.py", line 1775, in assemble operand.assemble(insn=insn, value=value) File "/home/ghostmansd/src/openpower-isa/src/openpower/decoder/power_insn.py", line 1035, in assemble value = int(value, 0) ValueError: invalid literal for int() with base 0: '.TOC.-.Lfunc_gep0@ha' 2. The pre-insndb branch is broken too: Traceback (most recent call last): File "/usr/local/bin/pysvp64asm", line 11, in <module> load_entry_point('libresoc-openpower-isa', 'console_scripts', 'pysvp64asm')() File "/home/ghostmansd/src/openpower-isa/src/openpower/sv/trans/svp64.py", line 1681, in asm_process lst = '; '.join(lst) File "/home/ghostmansd/src/openpower-isa/src/openpower/sv/trans/svp64.py", line 1127, in translate_one pmmode, pmask = decode_predicate(encmode[2:]) File "/home/ghostmansd/src/openpower-isa/src/openpower/sv/trans/svp64.py", line 700, in decode_predicate "encoding %s for predicate not recognised" % encoding AssertionError: encoding pred for predicate not recognised As for pure-binutils version, I've managed to make binutils build the assembly (this needed few fixes on binutils side). However, the test fails upon comparison: /tmp/out0 data/audio/mp3/mp3_1_data/out0 differ: char 1, line 1 make[1]: Leaving directory '/home/ghostmansd/src/openpower-isa/media' The assembly should be compared, though this will require some changes in the assembly. The progress is at media-binutils branch.
At least pre-insndb branch is fixed easily, so there's a way to compare its assembly with pure binutils.
OK, since svp64trans is not capable of tricks like `*ip+3`. We only support trivial macros, and such code will fail there until we address these tasks: https://bugs.libre-soc.org/show_bug.cgi?id=993 https://bugs.libre-soc.org/show_bug.cgi?id=994 As for crypto, `make -C crypto/chacha20/ BINDIR=/tmp` works for me, but I have no idea what to do with its artifacts.
(In reply to Dmitry Selyutin from comment #8) > OK, since svp64trans is not capable of tricks like `*ip+3`. nothing like that should have been added to any of the media/ mp3 assembler. it was written very early. > We only support > trivial macros, and such code will fail there until we address these tasks: > https://bugs.libre-soc.org/show_bug.cgi?id=993 > https://bugs.libre-soc.org/show_bug.cgi?id=994 note "linking" note in 994, these were utterly lost until now. linking means adding "See also", or "blocks", etc. > As for crypto, `make -C crypto/chacha20/ BINDIR=/tmp` works for me, but I > have no idea what to do with its artifacts. if it passes the unit tests then you didn't break anything :)
- sv.mulli/m=r10 *t2, *t, 2217 # t2 has c1 * 2217, d1 * 2217 - sv.mulli/m=r10 *t3, *t, 5352 # t3 has c1 * 5352, d1 * 5352 + sv.mulli/m=%r10 *t2, *t, 2217 # t2 has c1 * 2217, d1 * 2217 + sv.mulli/m=%r10 *t3, *t, 5352 # t3 has c1 * 5352, d1 * 5352 do you mind taking those percentage character out, i find them really irritating as i verbally "hear" them in my mind when looking at them. the extra time it takes to hear the words "comma PER-CENT arr ten" is a distraction.
(In reply to Dmitry Selyutin from comment #6) > As for pure-binutils version, I've managed to make binutils build the > assembly (this needed few fixes on binutils side). However, the test fails > upon comparison: > /tmp/out0 data/audio/mp3/mp3_1_data/out0 differ: char 1, line 1 > make[1]: Leaving directory '/home/ghostmansd/src/openpower-isa/media' > probably because the meaning of setvl changed recently. 83 # Use SETVL again as we want to store 18 floats to (out) 84 setvl 0,0,18,0,1,1 > > The assembly should be compared, though this will require some changes in > the assembly. can you put "r30" back in as "pred"? yes i know it looks like it wasn't used at lines 79 and 80 when it should have been. 21 .set pred, 30 79 li 30, 0 80 ori 30, 30, 0xaaaa # Predicate mask 0b1010101010101010 81 # equivalent to: for (i = 17; i >= 3; i -= 2) in[i] += in[i-2]; 82 sv.fadds/mrr/m=%r30 *vin2, *vin2, *vin1 that should at lines 79 and 80 be using "pred" as a macro, but hmm did we remember to allow "m=30"? if not: arrrgh :)
As for r30/30/*/%, here's that discussion: https://lists.libre-soc.org/pipermail/libre-soc-dev/2022-June/004996.html We already iterated through this. Either -regnames, or explicit registers. I think this might work with `.set pred,%r30`. But in predicates it always tries to look for register names.
I think I can make these work. The code we have for now looks like this: if ((exp.X_op == O_register) && ((exp.X_add_number == 3) || (exp.X_add_number == 10) || (exp.X_add_number == 30))) { exp.X_op = O_predicate; if (exp.X_add_number == 3) exp.X_add_number = SVP64_PREDICATE_R3; else if (exp.X_add_number == 10) exp.X_add_number = SVP64_PREDICATE_R10; else if (exp.X_add_number == 30) exp.X_add_number = SVP64_PREDICATE_R30; } However, this leads to ambiguity: what is implied, 30 or %r30? After all, this is why we sticked to ^r3 notation instead of (1<<r3), too.
BTW the discussion above is not complete; somewhere in the IRC we came up with asterisk for *. And also custom condition registers. if isinstance(insn, SVP64Instruction): yield f"{vector}cr{CR}.{cond}" else: yield f"4*cr{CR}+{cond}"
crypto/chacha20/test-chacha20 works with the recent binutils! As for media tests, I suggest that I merge media-binutils branch into master, since it certainly improves things (a breaking test is way better than a test which cannot even be built). Objections?
(In reply to Dmitry Selyutin from comment #12) > As for r30/30/*/%, here's that discussion: > https://lists.libre-soc.org/pipermail/libre-soc-dev/2022-June/004996.html > > We already iterated through this. Either -regnames, or explicit registers. I > think this might work with `.set pred,%r30`. But in predicates it always > tries to look for register names. that means pysvp64asm needs to support it too. much as i don't like % it's an ok path to get things to work
(In reply to Dmitry Selyutin from comment #15) > crypto/chacha20/test-chacha20 works with the recent binutils! hoorah! > As for media tests, I suggest that I merge media-binutils branch into > master, since it certainly improves things yeah go on :) > (a breaking test is way better > than a test which cannot even be built). Objections? naah
`.set pred,%r30` works, so I updated the patch and pushed it to master. In scope of this task, crypto test was checked, and media can be run again (with failure, so this should be inspected by authors). Is there any other stuff we'd like to cover in scope of this task?
(In reply to Dmitry Selyutin from comment #18) > `.set pred,%r30` works, GREAT. > so I updated the patch and pushed it to master. > In scope of this task, crypto test was checked, and media can be run again > (with failure, so this should be inspected by authors). > Is there any other stuff we'd like to cover in scope of this task? naah the binutils update was really enough on its own.