feat(sio): fix implemented Hardware Divider #37

Turro75 · 2021-05-21T18:59:38Z

thanks @kilograham for pointing me in the right direction

@kilograham

thanks @kilograham for pointing me in the right direction

urish · 2021-05-21T21:41:01Z

Thanks Valerio!

Yes, Graham surely knows the RP2040 much better than me. I'm so happy he's helping us :-)

Do you want us to add some unit tests for the divider in the next live stream? Or do you want to try adding them yourself?

test based on hello_divider from pico_examples

…040js into Hardware-Divider-fix

urish · 2021-05-22T08:47:34Z

src/sio.spec.ts

+const _DIV_SDIVISOR = SIO_START_ADDRESS + 0x06c; //  Divider signed divisor
+const _DIV_QUOTIENT = SIO_START_ADDRESS + 0x070; //  Divider result quotient
+const _DIV_REMAINDER = SIO_START_ADDRESS + 0x074; //Divider result remainder
+const _DIV_CSR = SIO_START_ADDRESS + 0x078;


Can you please remove the _ (underscore) in front of the constant? The idea is to keep this code consistent with the rest of the code

ok, I'm moving test to be gdbclient ready, I'll do it in the next commit.

Oh great. I was going to suggest that, but you are faster!

ok done,
I had to add several "| 0"which are needed when running on silicone and the read value is signed, maybe a readSInt32 implementation could be better? or maybe not as it matters only during test?

copied the silicone behavior when divisor=0 -> quot=-1, rem=dividend
I wasn't able to find any word related to div/0 exceptions in the datasheets, is this possible?

_DIV_XXX has been renamed to SIO_DIV_XXX, I choose this way as SIO_DIV_XXX = SIO_ADDRESS_BASE + DIV_XXX
Even if declared in different files, I wouldn't have the same name to both offset and full address.

Thank you!

copied the silicone behavior when divisor=0 -> quot=-1, rem=dividend

Good idea to test what happens when dividing by zero.

I wasn't able to find any word related to div/0 exceptions in the datasheets, is this possible?

No idea! Did you see it getting any exception on the silicone?

I had to add several "| 0"which are needed when running on silicone and the read value is signed, maybe a readSInt32 implementation could be better? or maybe not as it matters only during test?

I think this is a good idea, it will make the tests easier to read / understand. I suggest calling it readInt32() (without the S), similar to how they call the signed int array in JavaScript (and also C++ type int32_t vs uint32_t).

_DIV_XXX has been renamed to SIO_DIV_XXX, I choose this way as SIO_DIV_XXX = SIO_ADDRESS_BASE + DIV_XXX
Even if declared in different files, I wouldn't have the same name to both offset and full address.

👍

fix(sio):fix DIV_CSR register behavior test(sio):move to test driver for silicone/emu comparison

urish · 2021-05-22T19:21:28Z

src/sio.ts

@@ -117,6 +146,56 @@ export class RPSIO {
      case GPIO_HI_OE_XOR:
        this.qspiGpioOutputEnable ^= value & GPIO_MASK;
        break;
+      case DIV_UDIVIDEND:
+        this.divDividend = value >>> 0;
+        this.divCSR = 0b10;


what's the reason for updating divCSR twice in the same switch case?

I was waiting for You on this...
well the datasheet states that the CSR lsb is cleared at the beginning of the calculation and set once result is ready (after 8 cpu cycles).
On a single core this assignment does not make any sense, since I don't know how multicore will be implemented it could be something that will matter in case of concurrency against hardware divider.

That's a good point. When we get to multicore, we'll just execute one instruction at a time (so jumping back and forth between running core0 and core1 code).

In any case, JavaScript does not support multi-threading natively. The only way is using Web Workers (or worker threads on Node.js). But even then, the only way two threads can access the same memory is using a SharedArrayBuffer, so if we decide at some point to truly run this on two threads (and share the divider memory between them) we'll have to change all these variables to be stored inside a SharedArrayBuffer and refactor the the code, and I don't see it happening any time soon.

So to summarize, I think we can safely remove the this.divCSR = 0b10;

note that SIO dividers/interpreters are core local

That's a good point to remember when we implemented multi-core. I noted this!

urish · 2021-05-22T19:25:52Z

src/sio.ts

+      case DIV_SDIVISOR:
+        this.divCSR = 0b10;
+        this.divDivisor = value | 0;
+        if (this.divDivisor == 0) {


I suggest that we move this repetitive code (in DIV_UDIVIDEND, DIV_SDIVIDEND, DIV_UDIVISOR, and DIV_SDIVISOR) into a function that will do the actual division, e.g. this.updateDivider().

As far as I can tell, you don't need to worry about the sign of the result, because when it's read back into a register it'll always be internally converted to unsigned anyway.

urish · 2021-05-22T19:29:22Z

src/sio.spec.ts

+  });
+
+  describe('Hardware Divider', () => {
+    it('should set perform a signed hardware divider 123456 / -321 = -384 R192', async () => {


I think "REM 192" will be clearer than "R192" (same for the other test names).

Also, the word "set" (second word) seems not related here?

urish · 2021-05-22T19:30:37Z

src/sio.spec.ts

+      expect((await cpu.readUint32(SIO_DIV_QUOTIENT)) | 0).toEqual(-384);
+    });
+
+    it('should set perform an unsigned division by zero 123456 / 0 = Infinity RNaN', async () => {


The result isn't Infinity / NaN (as the test name says), rather 0xffffffff and remainder 123456.

adjusted test by using readInt32 where signed value are expected

urish

Thanks Valerio! I added a few comments / suggestions. I like the shape this is taking :)

src/sio.spec.ts

urish · 2021-05-23T10:37:18Z

src/utils/test-driver-gdb.ts

+
+  async readInt32(address: number) {
+    const result = await this.gdbClient.readMemory(address, 4);
+    return new Uint32Array(result.buffer)[0] | 0;


why not this.readUint32(address) | 0 like you did in the RP2040 test driver?

urish · 2021-05-23T10:38:41Z

src/sio.ts

@@ -147,46 +159,14 @@ export class RPSIO {
        this.qspiGpioOutputEnable ^= value & GPIO_MASK;
        break;
      case DIV_UDIVIDEND:
-        this.divDividend = value >>> 0;


Does it work correctly even though you use the same this.divDividend = value; for both the signed/unsigned cases (instead of value >>> 0 vs value | 0)?

in the end yes, it works in both cases.
I don't understand the reason to have separated registers, at the beginning I though putting a boolean flag to change the behavior between signed/unsigned, it turned out a useless handling.

Have you tried dividing -3000 by 2?

In JavaScript I get different results (printing both results as signed, just to show the difference):

const signedDiv = (a, b) => ((a|0)/(b|0))|0; const unsignedDiv = (a, b) => ((a>>>0)/(b>>>0))|0; console.log(signedDiv(-3000, 2)); // -1500 console.log(unsignedDiv(-3000, 2)); // 2147482148

I added this test, it passes on both emu and sil.
what I've seen is that in the emu it doesn't matter if I use readUint32 or readInt32 it always return the right value.
test in the silicon instead it changes and it requires the right read method.
Seems that javascript (math.trunc?) automatically handles the right way in case of negative value.

For the result, it shouldn't matter how you read it (you can convert between signed / unsigned at any stage).

But for the actual division, it seems to matter even if you use math.trunc:

const signedDiv = (a, b) => Math.trunc((a|0)/(b|0))|0; const unsignedDiv = (a, b) => Math.trunc((a>>>0)/(b>>>0))|0; console.log(signedDiv(-3000, 2)); // -1500 console.log(unsignedDiv(-3000, 2)); // 2147482148

It could be the the current code is always doing signed division, and that's why the test passes. In this case, you can add a test for unsigned divide as well, trying to divide 0x80000000 by 2 using unsigned integer divide, and verify that you get the correct result (0x40000000):

console.log((signedDiv(0x80000000, 2) >>> 0).toString(16)); // 0xc0000000 console.log((unsignedDiv(0x80000000, 2) >>> 0).toString(16)); // 0x40000000

arghhhh!!! missing break forced signed code.
I've also had to force the readUint32 to return unsigned to pass this test:
should perform a signed division 0x80000000 / 2 = 0xc0000000 REM 0

src/sio.spec.ts

src/sio.ts

…n behavior Improved division by zero behavior added some relevant test

src/sio.ts

…ration fix quotient value for all signed/unsigned cases added specific test case to check that actually signed/unsigned operation is performed fix on test-driver-rp2040: force readUint32 to return unsigned value

urish · 2021-05-23T22:45:35Z

Thanks Valerio, this looks good to me :-)

Is there anything else you want to add/change before I merge?

Turro75 · 2021-05-24T05:30:31Z

Oh no Uri, I think this divider took enough time. It is fine now.

urish · 2021-05-24T07:27:02Z

Alright, merged!

Thanks for working on it, it was a long one indeed... But now I feel the instruction set is truly complete (this hardware divider is like another CPU instruction). We'll review the implementation in the live stream tomorrow :-)

Turro75 · 2021-05-24T19:31:38Z

Hi Uri,

I want to say sorry for forcing You to stay so much time on this, I'm sure You would have successfully write it in less than 20 minutes.
I wasn't really aware of signed/unsigned trap of javascript (after many years on freepascal) and forgot to set relevant breaks in switch/case. My regrets are that mostly I sent many commits too early.
Now I'm PR spinlocks which instead I'm pretty sure the code is ready to be merged.

urish · 2021-05-24T20:31:18Z

Hi Valerio,

First of all, thanks for caring about my time! I'm very pleased with the results. Maybe just writing the code would take me 20 minutes, but as you've seen in the live streams, writing the test cases, finding the bugs + verifying with the silicone would probably take much more. Like the alarms last week took about 2 hours.

Anyway, all the bugs you've found so far with gdbdiff (and fixed) surely saved much precious time. And I'm happy to see your commits and provide feedback. This way everyone benefits :)

Turro75 added 2 commits May 20, 2021 21:43

feat(RP2040): implemented RESET_BASE peripherals

d810256

feat(sio): fix implemented Hardware Divider

1a6550c

thanks @kilograham for pointing me in the right direction

urish added the enhancement New feature or request label May 21, 2021

urish linked an issue May 21, 2021 that may be closed by this pull request

hello_timer wrong printf output #35

Closed

Turro75 and others added 5 commits May 22, 2021 00:47

feat(sio): add cycles count for hardware divider

e89f2f6

test(sio): add test for hardware divider

79925fa

test based on hello_divider from pico_examples

test(sio): add test for hardware divider

40694fb

test based on hello_divider from pico_examples

wrong test description fixed

6bfe234

Merge branch 'Hardware-Divider-fix' of https://github.com/Turro75/rp2…

c968ed8

…040js into Hardware-Divider-fix

urish reviewed May 22, 2021

View reviewed changes

fix(sio):fix unsigned/signed hardware divisor

a7e7e6a

fix(sio):fix DIV_CSR register behavior test(sio):move to test driver for silicone/emu comparison

urish requested changes May 22, 2021

View reviewed changes

Turro75 added 2 commits May 23, 2021 12:28

feat(test-driver): add readInt32 for signed responses

b7553f9

feat(hardware divider): moved case code to dedicate function

00e2c81

adjusted test by using readInt32 where signed value are expected

urish requested changes May 23, 2021

View reviewed changes

Turro75 added 4 commits May 23, 2021 13:46

feat(test-driver): align gdb readInt32 to rp2040 implementation

5583d49

fix(sio): update cycles and CSR even with div/0

88e7904

test(sio): added test -3000/2

5353e80

fix(hardware divider): changed division math to better reflect silico…

126c4c0

…n behavior Improved division by zero behavior added some relevant test

urish reviewed May 23, 2021

View reviewed changes

src/sio.ts Outdated Show resolved Hide resolved

urish merged commit 36f2e7c into wokwi:master May 24, 2021

Turro75 deleted the Hardware-Divider-fix branch May 24, 2021 22:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sio): fix implemented Hardware Divider #37

feat(sio): fix implemented Hardware Divider #37

Turro75 commented May 21, 2021

urish commented May 21, 2021

urish May 22, 2021

Turro75 May 22, 2021

urish May 22, 2021

Turro75 May 22, 2021

urish May 22, 2021 •

edited

Loading

urish May 22, 2021

Turro75 May 23, 2021

urish May 23, 2021

kilograham May 23, 2021

urish May 23, 2021

urish May 22, 2021

urish May 22, 2021

urish May 22, 2021

urish left a comment

urish May 23, 2021

urish May 23, 2021

Turro75 May 23, 2021

urish May 23, 2021

Turro75 May 23, 2021

urish May 23, 2021

Turro75 May 23, 2021

urish commented May 23, 2021

Turro75 commented May 24, 2021

urish commented May 24, 2021

Turro75 commented May 24, 2021

urish commented May 24, 2021

feat(sio): fix implemented Hardware Divider #37

feat(sio): fix implemented Hardware Divider #37

Conversation

Turro75 commented May 21, 2021

urish commented May 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urish May 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urish commented May 23, 2021

Turro75 commented May 24, 2021

urish commented May 24, 2021

Turro75 commented May 24, 2021

urish commented May 24, 2021

urish May 22, 2021 •

edited

Loading