MUL pipeline needs unit tests similar to the 5-deep nested loop test Jacob wrote for the DIV pipeline. https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/div/test/test_pipe_caller_long.py;hb=HEAD
Luke, you said I should run through a list of opcodes. Can this list of opcodes be found in the decoder? I started by looking in the 3.1B OPENPOWER spec, but there are lots of other variations such as "multiply low" "multiply high" there that I don't see in Jacob's test. I think that extracting each opcode individually *manually* from the PDF spec is going to be error prone, so I assume there's a different approach you would recommend?
(In reply to Cole Poirier from comment #1) > Luke, you said I should run through a list of opcodes. yes. look in the "long" div test. > Can this list of opcodes be found in the decoder? no - in the spec. > I started by looking in the 3.1B OPENPOWER > spec, but there are lots of other variations such as "multiply low" > "multiply high" there that I don't see in Jacob's test. exactly. > I think that > extracting each opcode individually *manually* from the PDF spec is going to > be error prone, then use the text file from the wiki. > so I assume there's a different approach you would recommend? no.
I may have missed some, but the complete list of instructions built into power-instruction-analyzer is accessible as: >>> print(power_instruction_analyzer.INSTRS) ['divde', 'divdeo', 'divde.', 'divdeo.', 'divdeu', 'divdeuo', 'divdeu.', 'divdeuo.', 'divd', 'divdo', 'divd.', 'divdo.', 'divdu', 'divduo', 'divdu.', 'divduo.', 'divwe', 'divweo', 'divwe.', 'divweo.', 'divweu', 'divweuo', 'divweu.', 'divweuo.', 'divw', 'divwo', 'divw.', 'divwo.', 'divwu', 'divwuo', 'divwu.', 'divwuo.', 'modsd', 'modud', 'modsw', 'moduw', 'mullw', 'mullwo', 'mullw.', 'mullwo.', 'mulhw', 'mulhw.', 'mulhwu', 'mulhwu.', 'mulld', 'mulldo', 'mulld.', 'mulldo.', 'mulhd', 'mulhd.', 'mulhdu', 'mulhdu.', 'maddhd', 'maddhdu', 'maddld']
(In reply to Luke Kenneth Casson Leighton from comment #2) > then use the text file from the wiki. This is the right approach, the method I was using was looking in the generated fixedarith.py, by looking at the wiki I got the MUL instructions that didn't start with mulxxx i.e. maddxxx. In order to set the test up to run, jacob created a helper.py file, this doesn't exist in the MUL test dir. Jacob also created a DivPipeKind enum because of the different DIV pipeline implementations. Overall I'm not sure how to properly use the api of the pipelines to write this test. If I commit what I have can you help me with this?
(In reply to Jacob Lifshay from comment #3) > I may have missed some, but the complete list of instructions built into > power-instruction-analyzer is accessible as: > > >>> print(power_instruction_analyzer.INSTRS) > ['divde', 'divdeo', 'divde.', 'divdeo.', 'divdeu', 'divdeuo', 'divdeu.', > 'divdeuo.', 'divd', 'divdo', 'divd.', 'divdo.', 'divdu', 'divduo', 'divdu.', > 'divduo.', 'divwe', 'divweo', 'divwe.', 'divweo.', 'divweu', 'divweuo', > 'divweu.', 'divweuo.', 'divw', 'divwo', 'divw.', 'divwo.', 'divwu', > 'divwuo', 'divwu.', 'divwuo.', 'modsd', 'modud', 'modsw', 'moduw', 'mullw', > 'mullwo', 'mullw.', 'mullwo.', 'mulhw', 'mulhw.', 'mulhwu', 'mulhwu.', > 'mulld', 'mulldo', 'mulld.', 'mulldo.', 'mulhd', 'mulhd.', 'mulhdu', > 'mulhdu.', 'maddhd', 'maddhdu', 'maddld'] Thanks Jacob, very helpful!
(In reply to Cole Poirier from comment #4) > (In reply to Luke Kenneth Casson Leighton from comment #2) > > then use the text file from the wiki. > > This is the right approach, the method I was using was looking in the > generated fixedarith.py, by looking at the wiki I got the MUL instructions > that didn't start with mulxxx i.e. maddxxx. > > In order to set the test up to run, jacob created a helper.py file, irrelevant. > this > doesn't exist in the MUL test dir. Jacob also created a DivPipeKind enum irrelevant. > because of the different DIV pipeline implementations. irrelevant. > Overall I'm not sure > how to properly use the api of the pipelines to write this test. irrelevant. > If I commit > what I have can you help me with this? it's real simple. from this file: https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/div/test/test_pipe_caller_long.py;hb=HEAD at this line number: https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/div/test/test_pipe_caller_long.py;hb=HEAD#l11 copy that function adapt it to do "mul" instead of "div". add it to this file: https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/mul/test/test_pipe_caller.py;hb=HEAD that's all that's needed. nothing more, nothing less.
(In reply to Luke Kenneth Casson Leighton from comment #6) > (In reply to Cole Poirier from comment #4) > > (In reply to Luke Kenneth Casson Leighton from comment #2) > > > then use the text file from the wiki. > > > > This is the right approach, the method I was using was looking in the > > generated fixedarith.py, by looking at the wiki I got the MUL instructions > > that didn't start with mulxxx i.e. maddxxx. > > > > In order to set the test up to run, jacob created a helper.py file, > > irrelevant. > > > this > > doesn't exist in the MUL test dir. Jacob also created a DivPipeKind enum > > irrelevant. > > > because of the different DIV pipeline implementations. > > irrelevant. > > > Overall I'm not sure > > how to properly use the api of the pipelines to write this test. > > irrelevant. > > > If I commit > > what I have can you help me with this? > > it's real simple. > > from this file: > > https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/div/test/ > test_pipe_caller_long.py;hb=HEAD > > at this line number: > > https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/div/test/ > test_pipe_caller_long.py;hb=HEAD#l11 > > copy that function > > adapt it to do "mul" instead of "div". > > add it to this file: > > https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/mul/test/ > test_pipe_caller.py;hb=HEAD > > that's all that's needed. > > nothing more, nothing less. Thanks! That is absolutely dead simple, very helpful!
Pushed my commit. However, getting a weird cryptic error: ``` {standard input}: Assembler messages: {standard input}:1: Error: missing operand Error in program: maddhd 3, 1, 2 Traceback (most recent call last): File "/home/colepoirier/src/soc/src/soc/simulator/program.py", line 86, in _assemble sys.exit(1) SystemExit: 1 During handling of the above exception, another exception occurred: Traceback (most recent call last): File "test_pipe_caller.py", line 330, in <module> suite.addTest(TestRunner(MulTestCase().test_data)) File "/home/colepoirier/src/soc/src/soc/fu/test/common.py", line 84, in __init__ v(self) File "test_pipe_caller.py", line 197, in case_all with Program(l, bigendian) as prog: File "/home/colepoirier/src/soc/src/soc/simulator/program.py", line 46, in __init__ self._assemble() File "/home/colepoirier/src/soc/src/soc/simulator/program.py", line 87, in _assemble self._link(outfile) File "/usr/lib/python3.7/tempfile.py", line 639, in __exit__ self.close() File "/usr/lib/python3.7/tempfile.py", line 646, in close self._closer.close() File "/usr/lib/python3.7/tempfile.py", line 583, in close unlink(self.name) FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmpe8odpld7.o' ```
(In reply to Cole Poirier from comment #8) > Pushed my commit. However, getting a weird cryptic error: > > ``` > {standard input}: Assembler messages: > {standard input}:1: Error: missing operand > Error in program: > maddhd 3, 1, 2 look at the definition for this operation. count the number of operands.
(In reply to Luke Kenneth Casson Leighton from comment #9) > (In reply to Cole Poirier from comment #8) > > Pushed my commit. However, getting a weird cryptic error: > > > > ``` > > {standard input}: Assembler messages: > > {standard input}:1: Error: missing operand > > Error in program: > > maddhd 3, 1, 2 > > look at the definition for this operation. count the number of operands. ah that one appears to take 4... Not so simple after all. I don't have an idea as to how to fix this.
(In reply to Cole Poirier from comment #10) > > look at the definition for this operation. count the number of operands. > > ah that one appears to take 4... Not so simple after all. I don't have an > idea as to how to fix this. that's also very simple: 1) leave it out 2) put it in a separate test.
(In reply to Luke Kenneth Casson Leighton from comment #11) > (In reply to Cole Poirier from comment #10) > > > > look at the definition for this operation. count the number of operands. > > > > ah that one appears to take 4... Not so simple after all. I don't have an > > idea as to how to fix this. > > that's also very simple: > > 1) leave it out > > 2) put it in a separate test. Sure... I'd like to understand how to create this separate unit test. But I'm not sure what the correct api is for adding registers, I looked in the other MUL and DIV tests and they are all two operand, otherwise I would have just used one such test as a template.
(In reply to Cole Poirier from comment #12) > Sure... I'd like to understand how to create this separate unit test. But > I'm not sure what the correct api is for adding registers, I looked in the > other MUL and DIV tests and they are all two operand, otherwise I would have > just used one such test as a template. leave it. the fact that we have to discuss it is impeding your progress in committing a working test for everything else.
(In reply to Luke Kenneth Casson Leighton from comment #13) > (In reply to Cole Poirier from comment #12) > > > Sure... I'd like to understand how to create this separate unit test. But > > I'm not sure what the correct api is for adding registers, I looked in the > > other MUL and DIV tests and they are all two operand, otherwise I would have > > just used one such test as a template. > > leave it. the fact that we have to discuss it is impeding your progress > in committing a working test for everything else. Makes sense, I commented it out and added a TODO. Unfortunately the test fails like this. Should I commit? ``` ['RA'] reading reg 1 [SelectableInt(value=0x0, bits=64)] concat 128 SelectableInt(value=0x0, bits=128) abs 0 abs 0 SelectableInt mul 0x0 0x2 64 256 MULS SelectableInt(value=0x0, bits=320) False False F ====================================================================== FAIL: run_all (__main__.TestRunner) ---------------------------------------------------------------------- Traceback (most recent call last): File "test_pipe_caller.py", line 289, in run_all sim.run() File "/home/colepoirier/src/nmigen/nmigen/sim/pysim.py", line 382, in run while self.advance(): File "/home/colepoirier/src/nmigen/nmigen/sim/pysim.py", line 371, in advance self._real_step() File "/home/colepoirier/src/nmigen/nmigen/sim/pysim.py", line 347, in _real_step process.run() File "/home/colepoirier/src/nmigen/nmigen/sim/_pycoro.py", line 120, in run self.coroutine.throw(exn) File "/home/colepoirier/src/nmigen/nmigen/sim/_pycoro.py", line 61, in run command = self.coroutine.send(response) File "/home/colepoirier/src/nmigen/nmigen/sim/pysim.py", line 265, in wrapper yield from process() File "test_pipe_caller.py", line 273, in process yield from sim.call(opname) File "/home/colepoirier/src/soc/src/soc/decoder/isa/caller.py", line 668, in call results = info.func(self, *inputs) File "/home/colepoirier/src/soc/src/soc/decoder/isa/caller.py", line 766, in decorator result = func(*args, **kwargs) File "/home/colepoirier/src/soc/src/soc/decoder/isa/fixedarith.py", line 280, in op_mulli prod[0:128] = MULS(RA, EXTS(SI)) File "/home/colepoirier/src/soc/src/soc/decoder/selectable_int.py", line 311, in __setitem__ assert value.bits == bits, "%d into %d" % (value.bits, bits) AssertionError: 320 into 128 ---------------------------------------------------------------------- Ran 1 test in 2.526s FAILED (failures=1)
(In reply to Cole Poirier from comment #14) > Makes sense, I commented it out and added a TODO. Unfortunately the test > fails like this. Should I commit? you shouldn't be asking, it should have been done already: you should be *telling* me it's committed (2 days ago) within under 3 minutes. 2 days of uncommitted work is a major "red flag" because during all that time absolutely nobody can look at it or comment on it. the only reason i say this however is because unit tests are *SPECIFICALLY* isolated from all other tests and are consequently NOT on a critical code path that could prevent or prohibit anyone else from doing any work.
(In reply to Luke Kenneth Casson Leighton from comment #15) > (In reply to Cole Poirier from comment #14) > > > Makes sense, I commented it out and added a TODO. Unfortunately the test > > fails like this. Should I commit? > > you shouldn't be asking, it should have been done already: you should > be *telling* me it's committed (2 days ago) within under 3 minutes. > > 2 days of uncommitted work is a major "red flag" because during all > that time absolutely nobody can look at it or comment on it. > > the only reason i say this however is because unit tests are *SPECIFICALLY* > isolated from all other tests and are consequently NOT on a critical code > path that could prevent or prohibit anyone else from doing any work. Thanks for the clarification. I'm still far too afraid of introducing things that break things, so when I get errors I cower ;-) Also, I committed the initial test where it failed due to madd* insns yesterday. So only a half failure there :) Committed and pushed.
(In reply to Cole Poirier from comment #16) > Thanks for the clarification. I'm still far too afraid of introducing things > that break things, so when I get errors I cower ;-) python unit test infrasttucture is specifically designed to be isolated. > > Also, I committed the initial test where it failed due to madd* insns > yesterday. So only a half failure there :) > > Committed and pushed. sorted. git pull diff --git a/src/soc/decoder/helpers.py b/src/soc/decoder/helpers.py index 17534800..870ec4ab 100644 --- a/src/soc/decoder/helpers.py +++ b/src/soc/decoder/helpers.py @@ -4,6 +4,7 @@ from nmutil.divmod import trunc_divs, trunc_rems from operator import floordiv, mod from soc.decoder.selectable_int import selectltu as ltu from soc.decoder.selectable_int import selectgtu as gtu +from soc.decoder.selectable_int import check_extsign trunc_div = floordiv trunc_rem = mod @@ -37,6 +38,9 @@ def EXTS64(value): # signed version of MUL def MULS(a, b): + if isinstance(b, int): + b = SelectableInt(b, self.bits) + b = check_extsign(self, b) a_s = a.value & (1 << (a.bits-1)) != 0 b_s = b.value & (1 << (b.bits-1)) != 0 result = abs(a) * abs(b)
(In reply to Luke Kenneth Casson Leighton from comment #17) > (In reply to Cole Poirier from comment #16) > > > Thanks for the clarification. I'm still far too afraid of introducing things > > that break things, so when I get errors I cower ;-) > > python unit test infrasttucture is specifically designed to be isolated. Happy to be challenging myself again by engaging with you on the codebase, it's a pleasant discomfort of learning new things :) > > Also, I committed the initial test where it failed due to madd* insns > > yesterday. So only a half failure there :) > > > > Committed and pushed. > > sorted. git pull > > diff --git a/src/soc/decoder/helpers.py b/src/soc/decoder/helpers.py > index 17534800..870ec4ab 100644 > --- a/src/soc/decoder/helpers.py > +++ b/src/soc/decoder/helpers.py > @@ -4,6 +4,7 @@ from nmutil.divmod import trunc_divs, trunc_rems > from operator import floordiv, mod > from soc.decoder.selectable_int import selectltu as ltu > from soc.decoder.selectable_int import selectgtu as gtu > +from soc.decoder.selectable_int import check_extsign > > trunc_div = floordiv > trunc_rem = mod > @@ -37,6 +38,9 @@ def EXTS64(value): > > # signed version of MUL > def MULS(a, b): > + if isinstance(b, int): > + b = SelectableInt(b, self.bits) > + b = check_extsign(self, b) > a_s = a.value & (1 << (a.bits-1)) != 0 > b_s = b.value & (1 << (b.bits-1)) != 0 > result = abs(a) * abs(b) Thanks Luke! What should I do next?
(In reply to Cole Poirier from comment #18) > Thanks Luke! What should I do next? https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/mul/test/test_pipe_caller.py;h=3274f012794ffa6af1ee581a316805cdb7004ee6;hb=fb07d936ebbe71a4c1c9d67075b65955d8137dfc#l177 extend that list to include at least 0xffffffff 0x7fffffff 0x80000000 0xfffffffe 0xfffffffd these are in 32 bit close to the boundaries. also use that exact same list but this time use it for RA but for RB use a *random* value. also it might be worth looking at riscv isa tests to see what values they use. we also need that one where close to overflow is calculated by selecting an result in advance, divide by a selected RA (in python that is) and put *that* as RB.
(In reply to Luke Kenneth Casson Leighton from comment #19) > (In reply to Cole Poirier from comment #18) > > > Thanks Luke! What should I do next? > > https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/mul/test/test_pipe_caller.py;h=3274f012794ffa6af1ee581a316805cdb7004ee6;hb=fb07d936ebbe71a4c1c9d67075b65955d8137dfc#l177 > > extend that list to include at least the last 4 values in the list in soc.git are close to 32-bit boundaries when truncated to 32-bits. > 0xffffffff > 0x7fffffff > 0x80000000 > 0xfffffffe > 0xfffffffd only the last 2 are new when interpreting as 32-bit values.
(In reply to Jacob Lifshay from comment #20) > (In reply to Luke Kenneth Casson Leighton from comment #19) > > (In reply to Cole Poirier from comment #18) > > > > > Thanks Luke! What should I do next? > > > > https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/mul/test/test_pipe_caller.py;h=3274f012794ffa6af1ee581a316805cdb7004ee6;hb=fb07d936ebbe71a4c1c9d67075b65955d8137dfc#l177 > > > > extend that list to include at least > > the last 4 values in the list in soc.git are close to 32-bit boundaries when > truncated to 32-bits. > > > 0xffffffff > > 0x7fffffff > > 0x80000000 > > 0xfffffffe > > 0xfffffffd > > only the last 2 are new when interpreting as 32-bit values. 0xfffffffe is also already in soc.git when truncating to 32-bit values.
(In reply to Jacob Lifshay from comment #21) > (In reply to Jacob Lifshay from comment #20) > > (In reply to Luke Kenneth Casson Leighton from comment #19) > > > (In reply to Cole Poirier from comment #18) > > > > > > > Thanks Luke! What should I do next? > > > > > > https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/mul/test/test_pipe_caller.py;h=3274f012794ffa6af1ee581a316805cdb7004ee6;hb=fb07d936ebbe71a4c1c9d67075b65955d8137dfc#l177 > > > > > > extend that list to include at least > > > > the last 4 values in the list in soc.git are close to 32-bit boundaries when > > truncated to 32-bits. > > > > > 0xffffffff > > > 0x7fffffff > > > 0x80000000 > > > 0xfffffffe > > > 0xfffffffd > > > > only the last 2 are new when interpreting as 32-bit values. > > 0xfffffffe is also already in soc.git when truncating to 32-bit values. So, which ones do I actually need to add?
(In reply to Luke Kenneth Casson Leighton from comment #19) > also use that exact same list but this time use it for RA but for RB use a > *random* value. Done. Pushed. > also it might be worth looking at riscv isa tests to see what values they > use. Will take a look at this while I wait for clarification of the above values to be added. > we also need that one where close to overflow is calculated by selecting an > result in advance, divide by a selected RA (in python that is) and put > *that* as RB. I'm sorry I don't understand what you're directing me to do here, can you elaborate?
(In reply to Cole Poirier from comment #22) > So, which ones do I actually need to add? go ahead and add them all, it never hurts to test more cases, even if they are partially redundant.
(In reply to Jacob Lifshay from comment #24) > (In reply to Cole Poirier from comment #22) > > So, which ones do I actually need to add? > > go ahead and add them all, it never hurts to test more cases, even if they > are partially redundant. Gotcha, done, thanks Jacob :)
i just realised, mulli needs to be taken out of this "all" unit test and done separately and specially. the values need to be in the range that the constant SI can cope with. that is -32768 to +32767. however RA should be all those list of values 0x0000... etc
(In reply to Luke Kenneth Casson Leighton from comment #26) > i just realised, mulli needs to be taken out of this "all" unit test and > done separately and specially. > > > the values need to be in the range that the constant SI can cope with. that > is -32768 to +32767. > > however RA should be all those list of values 0x0000... etc Done, added a TODO for this. In an earlier commit I added a TODO for madd* 3 operand isns test.
(In reply to Jacob Lifshay from comment #21) > 0xfffffffe is also already in soc.git when truncating to 32-bit values. yeessumm... it is but it isn't quite the same. *we* know that some of the muls will truncate but it's not quite safe to assume that. one of the deep joys of corner/offbyone cases
(In reply to Cole Poirier from comment #27) > Done, added a TODO for this. good idea > In an earlier commit I added a TODO for madd* 3 > operand isns test. we have to implement madd first.
(In reply to Luke Kenneth Casson Leighton from comment #29) > (In reply to Cole Poirier from comment #27) > > > Done, added a TODO for this. > > good idea > > > In an earlier commit I added a TODO for madd* 3 > > operand isns test. > > we have to implement madd first. Right, do you want me to do that? Otherwise, which unit test should I write next? This one? "we also need that one where close to overflow is calculated by selecting an result in advance, divide by a selected RA (in python that is) and put *that* as RB." or this one? "i just realised, mulli needs to be taken out of this "all" unit test and done separately and specially. the values need to be in the range that the constant SI can cope with. that is -32768 to +32767. however RA should be all those list of values 0x0000... etc" or should I try adding some tests from the riscv unit tests?
(In reply to Cole Poirier from comment #23) > I'm sorry I don't understand what you're directing me to do here, can you > elaborate? x = 0x7fffffff # plus some small randomness ra = random value rb = x // ra # integer divide put ra and rb into regs 1 and 2 very simple. what this is testing is: *when* you multiply 2 numbers that *might* overflow, does it overflow, yes or no. if you tried with just a random ra and a random rb you would never hit 0x7ffffff not with millions of tests. so calculate it.
(In reply to Cole Poirier from comment #30) > > we have to implement madd first. > > Right, do you want me to do that? no, do tests first. also see if it ia in microwatt. if so raise separate bugreport. > Otherwise, which unit test should I write next? pick one.
(In reply to Luke Kenneth Casson Leighton from comment #31) > (In reply to Cole Poirier from comment #23) > > > I'm sorry I don't understand what you're directing me to do here, can you > > elaborate? > > x = 0x7fffffff # plus some small randomness > > ra = random value > > rb = x // ra # integer divide > > put ra and rb into regs 1 and 2 > > very simple. > > what this is testing is: *when* you multiply 2 numbers that *might* > overflow, does it overflow, yes or no. > > if you tried with just a random ra and a random rb you would never hit > 0x7ffffff not with millions of tests. > > so calculate it. Done :)
(In reply to Luke Kenneth Casson Leighton from comment #32) > (In reply to Cole Poirier from comment #30) > > > > we have to implement madd first. > > > > Right, do you want me to do that? > > no, do tests first. also see if it ia in microwatt. if so raise separate > bugreport. > > > Otherwise, which unit test should I write next? > > pick one. Am I able to implement the mulli special case test from just this? "the values need to be in the range that the constant SI can cope with. that is -32768 to +32767. however RA should be all those list of values 0x0000... etc"
(In reply to Cole Poirier from comment #33) > > if you tried with just a random ra and a random rb you would never hit > > 0x7ffffff not with millions of tests. > > > > so calculate it. > > Done :) great, will look tomorrow. (In reply to Cole Poirier from comment #34) > Am I able to implement the mulli special case test from just this? > > "the values need to be in the range that the constant SI can cope with. > that is -32768 to +32767. > however RA should be all those list of values 0x0000... etc" yes. however also add -32768, -32767, -2, -1, 0, 1, 2, 32766, 32767 as well as a few random values to the SI list.
(In reply to Luke Kenneth Casson Leighton from comment #35) > (In reply to Cole Poirier from comment #33) > > Done :) > > great, will look tomorrow. Cool. > (In reply to Cole Poirier from comment #34) > > > Am I able to implement the mulli special case test from just this? > > > > "the values need to be in the range that the constant SI can cope with. > > that is -32768 to +32767. > > however RA should be all those list of values 0x0000... etc" > > yes. however also add -32768, -32767, -2, -1, 0, 1, 2, 32766, 32767 as well > as a few random values to the SI list. Helpful, will do.
https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/mul/test/test_pipe_caller.py;hb=HEAD#l274 so... ra and rb are set once and then the exact same values issued to each instruction? that doesn't sound useful, does it? in addition: rb is in the range 0 to 2^64 while result is in the range 0 to 2^31. what is the probability that a small random number divided by a massively large random number will be anything other than zero? programming is about walking through the code and modelling its behaviour in your head. if you can't model it, you run it and explore the results. did you run this test and inspect the results? you would have seen every single instruction had rb=0, which is not the intent, is it?
(In reply to Luke Kenneth Casson Leighton from comment #37) > https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/mul/test/ > test_pipe_caller.py;hb=HEAD#l274 > > so... ra and rb are set once and then the exact same values issued to each > instruction? that doesn't sound useful, does it? > > in addition: rb is in the range 0 to 2^64 while result is in the range 0 to > 2^31. > > what is the probability that a small random number divided by a massively > large random number will be anything other than zero? Very, very helpful, I now much better understand how to write this test. Just committed a new version, please provide further feedback :) > programming is about walking through the code and modelling its behaviour in > your head. > > if you can't model it, you run it and explore the results. > > did you run this test and inspect the results? > > you would have seen every single instruction had rb=0, which is not the > intent, is it? Agreed, I just didn't understand it sufficiently to be able to do this, but these comments bring me closer to understanding. This was my intention. Commit what I understood I should do, and wait for better understanding to come through your corrections.
(In reply to Cole Poirier from comment #38) > Just committed a new version, please provide further feedback :) i would be able to do so if you had done "git push"... > > you would have seen every single instruction had rb=0, which is not the > > intent, is it? > > Agreed, I just didn't understand it sufficiently to be able to do this, but > these comments bring me closer to understanding. This was my intention. > Commit what I understood I should do, and wait for better understanding to > come through your corrections. now all we need to do is have _you_ think through the implications of what you're doing rather than asking me to do it! there are two absolutely crucial aspects to team programming: 1) communication - constant feedback and discussion 2) modelling and debugging: feedback on the *code*. the first of these you're doing extremely well. the latter you've not yet got the idea that *you* need to think through, from words/thoughts and abstract design concepts, translating that into step-by-step code. the expectation is therefore in this case that *each* instruction will need testing with a series of random values targetting the overflow range. that therefore implies a *two* level nested for-loop: outer loop: instruction inner loop: 20 (or so) execution: *NOW* we create random values targetting close to the overflow range.
(In reply to Luke Kenneth Casson Leighton from comment #39) > (In reply to Cole Poirier from comment #38) > > > Just committed a new version, please provide further feedback :) > > i would be able to do so if you had done "git push"... > > > > you would have seen every single instruction had rb=0, which is not the > > > intent, is it? > > > > Agreed, I just didn't understand it sufficiently to be able to do this, but > > these comments bring me closer to understanding. This was my intention. > > Commit what I understood I should do, and wait for better understanding to > > come through your corrections. > > now all we need to do is have _you_ think through the implications of what > you're doing rather than asking me to do it! That's what's happening now, I really wasn't capable of doing this prior. The next test I *will* be able to think through it my self as now I actually understand what's going on :) > there are two absolutely crucial aspects to team programming: > > 1) communication - constant feedback and discussion > 2) modelling and debugging: feedback on the *code*. > > the first of these you're doing extremely well. the latter you've not yet > got the idea that *you* need to think through, from words/thoughts and > abstract design concepts, translating that into step-by-step code. Got it now, looked through the results of the test this time, and they seem to be doing what they're supposed to do. > the expectation is therefore in this case that *each* instruction will need > testing with a series of random values targetting the overflow range. that > therefore implies a *two* level nested for-loop: > > outer loop: instruction > inner loop: 20 (or so) > execution: *NOW* we create random values targetting close to the overflow > range. Indeed, thank you, I am *really* feeling like I have a new fundamental understanding of how the tests work!
test_values = [-32768, -32767, -32766, -2, -1, 0, 1, 2, 32766, 32767, random.randint(-1 << 15, (1 << 15)-1), random.randint(-1 << 15, ( 1 << 15) - 1), random.randint(-1 << 15, (1 << 15)-1), random.randint(-1 << 15, (1 << 15)-1), random.randint(-1 << 15, (1 << 15)-1)] 80 chars per line please. limit the editor window to exactly 80 chars and there will not be a problem. also before committing do "git diff" - in an 80 char xterm - and again you will immediately spot which lines "wrap" because along the left hand side there will be spaces or characters instead of "+" or "-".
(In reply to Luke Kenneth Casson Leighton from comment #41) > test_values = [-32768, -32767, -32766, -2, -1, 0, 1, 2, 32766, > 32767, random.randint(-1 << 15, (1 << 15)-1), random.randint(-1 << 15, ( > 1 << 15) - 1), random.randint(-1 << 15, (1 << 15)-1), > random.randint(-1 << 15, (1 << 15)-1), random.randint(-1 << 15, (1 << 15)-1)] > > > > 80 chars per line please. limit the editor window to exactly 80 chars and > there will not be a problem. > > also before committing do "git diff" - in an 80 char xterm - and again > you will immediately spot which lines "wrap" because along the left hand > side there will be spaces or characters instead of "+" or "-". Yes I'm having trouble with this, I set my max line length to 80 chars, and ran autopep8 on it but I end up having the above atrocious formatting? Isn't autopep8 supposed to enforce 80 chars? I *can* manually review *every* time in an 80 char wide xterm, but would rather have my tools work properly. Please help.
(In reply to Cole Poirier from comment #42) > Yes I'm having trouble with this, I set my max line length to 80 chars, and > ran autopep8 on it but I end up having the above atrocious formatting? then you should have spotted that in the "git diff" prior to commit and fixed it! > Isn't autopep8 supposed to enforce 80 chars? only if you set -a -a -a. > I *can* manually review *every* time in an 80 char wide xterm, yes. do this, please, every time. it's what is expected. you cannot just "arbitrarily commit without thought and proper review". in particular: tools often do "damage". it's therefore critically important to do a full review. every single time.
(In reply to Luke Kenneth Casson Leighton from comment #43) > (In reply to Cole Poirier from comment #42) > > > Yes I'm having trouble with this, I set my max line length to 80 chars, and > > ran autopep8 on it but I end up having the above atrocious formatting? > > then you should have spotted that in the "git diff" prior to commit and > fixed it! Yes I *should* have, will do so going forwards. > > Isn't autopep8 supposed to enforce 80 chars? > > only if you set -a -a -a. That perplexes me, I would think it should be the default. Oh well. > > I *can* manually review *every* time in an 80 char wide xterm, > > yes. do this, please, every time. it's what is expected. you cannot just > "arbitrarily commit without thought and proper review". > > in particular: tools often do "damage". it's therefore critically important > to do a full review. > > every single time. Will do :)
https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/mul/test/test_pipe_caller.py;hb=HEAD#l290 blegh. that's a mess, Cole. instead, do a for loop appending one random value to a static list, each time.
l = ["mulli 3, 1, 2"] that is "please multiply register 1 by the constant 2, putting the result in register 3" this is not what we want.
(In reply to Luke Kenneth Casson Leighton from comment #46) > l = ["mulli 3, 1, 2"] > > that is "please multiply register 1 by the constant 2, putting the result in > register 3" > > this is not what we want. Sorry that's what's in all the other tests, what is it that we actually want?
(In reply to Luke Kenneth Casson Leighton from comment #45) > https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/mul/test/ > test_pipe_caller.py;hb=HEAD#l290 > > blegh. that's a mess, Cole. > > instead, do a for loop appending one random value to a static list, each > time. Like this? def case_mulli(self): test_values = [-32768, -32767, -32766, -2, -1, 0, 1, 2, 32766, 32767] for i in range(20): test_values.append(random.randint(-1 << 15, (1 << 15)-1)) l = ["mulli 3, 1, 2"] for ra in test_values: for rb in test_values: initial_regs = [0] * 32 initial_regs[1] = ra initial_regs[2] = rb # use "with" so as to close the files used with Program(l, bigendian) as prog: self.add_case(prog, initial_regs)
(In reply to Cole Poirier from comment #47) > Sorry that's what's in all the other tests, all the ones that take *registers* > what is it that we actually want? look in the 3.0B manual how mulli works (or the pseudocode in fixedarith.mdwn) then also look in alu test or anywhere that "addi" is used.
(In reply to Luke Kenneth Casson Leighton from comment #49) > (In reply to Cole Poirier from comment #47) > > > Sorry that's what's in all the other tests, > > all the ones that take *registers* Ahhh right, it's an immediate op > > what is it that we actually want? > > look in the 3.0B manual how mulli works (or the pseudocode in > fixedarith.mdwn) > > then also look in alu test or anywhere that "addi" is used. Very helpful, thanks.
I updated (and pushed) mulli to what I thought would be the way to do it but I know I'm still doing it wrong... because the tests only have register 'ra' and it's always zero. I don't know how to resolve this from looking at addi's use in the other unit tests.
(In reply to Cole Poirier from comment #51) > I updated (and pushed) mulli to what I thought would be the way to do it but > I know I'm still doing it wrong... because the tests only have register 'ra' > and it's always zero. you're looking at the output and finding discrepancies from expected behaviour. good. > I don't know how to resolve this from looking at > addi's use in the other unit tests. choice = random.choice(test_values) l = [f"mulli 1, 0, {choice}"] this says, "multiply register 0 by the immediate X and put the result in register 1, doesn't it? initial_regs = [0] * 32 initial_regs[1] = random.randint(-1 << 15, (1 << 15) - 1) initial_regs[2] = random.randint(-1 << 15, (1 << 15) - 1) here, register 1 and 2 have been set to a random value. what is register 0 set to by the previous line? so why are you surprised to find that exactly what you have asked to be done is in fact being done?
(In reply to Luke Kenneth Casson Leighton from comment #52) > (In reply to Cole Poirier from comment #51) > > I updated (and pushed) mulli to what I thought would be the way to do it but > > I know I'm still doing it wrong... because the tests only have register 'ra' > > and it's always zero. > > you're looking at the output and finding discrepancies from expected > behaviour. good. Learning. > > I don't know how to resolve this from looking at > > addi's use in the other unit tests. > > choice = random.choice(test_values) > l = [f"mulli 1, 0, {choice}"] > > this says, "multiply register 0 by the immediate X and put the result in > register 1, doesn't it? Apparently that's what it says. Very helpful, I was struggling with this. Going to go through point by point to confirm my understanding, below. > > initial_regs = [0] * 32 So this sets r0 through r31 to 0? Registers are 64 bits? > initial_regs[1] = random.randint(-1 << 15, (1 << 15) - 1) > initial_regs[2] = random.randint(-1 << 15, (1 << 15) - 1) > > here, register 1 and 2 have been set to a random value. Right. > what is register 0 set to by the previous line? It is set to 0 because that's what all registers are initialized to. > so why are you surprised to find that exactly what you have asked to be done > is in fact being done? Because I don't understand how to read it fluently yet! But I'm learning quite a lot just today. Thanks so much Luke, I'm beginning to understand.
(In reply to Cole Poirier from comment #53) > > Going to go through point by point to confirm my understanding, below. this is programming. one line at a time. > > > > initial_regs = [0] * 32 > > So this sets r0 through r31 to 0? it initialises an array which, when handed to ISACaller, does that, yes > Registers are 64 bits? yyup. > > what is register 0 set to by the previous line? > > It is set to 0 because that's what all registers are initialized to. and you asked for register 0, which was set to zero, to be RA.
(In reply to Luke Kenneth Casson Leighton from comment #54) > (In reply to Cole Poirier from comment #53) > this is programming. one line at a time. Indeed. > > > initial_regs = [0] * 32 > > > > So this sets r0 through r31 to 0? > > it initialises an array which, when handed to ISACaller, does that, yes Ah ISACaller, will be getting to know this module well. > > Registers are 64 bits? > > yyup. I was 99% sure, but had a niggling doubt. > > > what is register 0 set to by the previous line? > > > > It is set to 0 because that's what all registers are initialized to. Sorry, all registers are initialized to by the line "initial_regs = [0] * 32", well eventually as I now understand this is an array that specifies the initial values for ISACaller to set. > and you asked for register 0, which was set to zero, to be RA. Right, because: mulli RT,RA,SI and I asked for "mulli 1 0 X" therefore: RT = r1 RA = r0 X = immediate (random.choice from test_values)
Updated and pushed, I think it actually works from examining the test output!
(In reply to Cole Poirier from comment #56) > Updated and pushed, I think it actually works from examining the test output! test_values = [-32768, -32767, -32766, -2, -1, 0, 1, 2, 32766, 32767] so, this is now some fixed values (only), where we want fixed values *and* random values appended. for i in range(40): choice = random.choice(test_values) l = [f"mulli 0, 1, {choice}"] this is now "multiply immediate-choice randomly selected by register 1 and put the result in register 0" we don't want "a random choice" we want a for-loop of *all* options from test_values initial_regs = [0] * 32 initial_regs[1] = random.randint(-1 << 15, (1 << 15) - 1) this - a 64-bit register - is limited to the range -32768 to +32767 we want register 1 to be the full range 0 to 2^64-1
(In reply to Luke Kenneth Casson Leighton from comment #57) > (In reply to Cole Poirier from comment #56) > > Updated and pushed, I think it actually works from examining the test output! > > test_values = [-32768, -32767, -32766, -2, -1, 0, 1, 2, 32766, > 32767] > > so, this is now some fixed values (only), where we want fixed values > *and* random values appended. > > for i in range(40): > choice = random.choice(test_values) > l = [f"mulli 0, 1, {choice}"] > > this is now "multiply immediate-choice randomly selected by register 1 > and put the result in register 0" > > we don't want "a random choice" we want a for-loop of *all* options from > test_values Right, that makes more sense. I'm sorry but I wasn't able to figure out the manner in which you want me to append these random values? Is it simple a line before the "for val in test_values" loop that goes like this? ``` for i in range(20): test_values.append(random.randint(-1 << 15, (1 << 15) - 1)) ``` > initial_regs = [0] * 32 > initial_regs[1] = random.randint(-1 << 15, (1 << 15) - 1) > > this - a 64-bit register - is limited to the range -32768 to +32767 > we want register 1 to be the full range 0 to 2^64-1 Thank you, I was under the misapprehension that it needed to be in the 2^16-1 range in order to ever hit the edge cases over million of runs, thanks for helping me clear up this misunderstanding.
(In reply to Cole Poirier from comment #58) > (In reply to Luke Kenneth Casson Leighton from comment #57) > > (In reply to Cole Poirier from comment #56) > > > Updated and pushed, I think it actually works from examining the test output! > > > > test_values = [-32768, -32767, -32766, -2, -1, 0, 1, 2, 32766, > > 32767] > > > > so, this is now some fixed values (only), where we want fixed values > > *and* random values appended. > > > > for i in range(40): > > choice = random.choice(test_values) > > l = [f"mulli 0, 1, {choice}"] > > > > this is now "multiply immediate-choice randomly selected by register 1 > > and put the result in register 0" > > > > we don't want "a random choice" we want a for-loop of *all* options from > > test_values > > Right, that makes more sense. I'm sorry but I wasn't able to figure out the > manner in which you want me to append these random values? Is it simple a > line before the "for val in test_values" loop that goes like this? > > ``` > for i in range(20): > test_values.append(random.randint(-1 << 15, (1 << 15) - 1)) > ``` yep that's it. the idea is to tidy up the morass of dozens of hand-added random values, not change the actual functionality. > > initial_regs = [0] * 32 > > initial_regs[1] = random.randint(-1 << 15, (1 << 15) - 1) > > > > this - a 64-bit register - is limited to the range -32768 to +32767 > > we want register 1 to be the full range 0 to 2^64-1 > > Thank you, I was under the misapprehension that it needed to be in the > 2^16-1 range in order to ever hit the edge cases over million of runs, shrinking the range definitely won't achieve that. > thanks for helping me clear up this misunderstanding. i see where you're heading, good point, yes we can refine it, by combining similar things to the "all" test. however it should be the 0x0 0x1 0xffff.. etc etc list again expanded by some random values this time in the range 0 to 2^64-1 so the inner loop should set up the list same as in the "all" test named test_values_ra, append a few random values, then do "for ra in test_values_ra"