Enable dynamic-sized RegisterBits #515

furuame · 2024-08-29T09:43:33Z

Hello Knute,

In some vector processors, we may need registers longer than 512-bit. Was std::array used for considering C++ performance? It might work if I just upsize it to 1024-bit, but I think there could be larger ones in the future. Use std::vector instead could be once and for all.

Let me know your thoughts! I tried to make 64 as template parameter by default -- But Registerbits seems to be used heavily as return value type, and class template argument deduction can’t work for that case.

klingaard · 2024-08-29T16:22:33Z

Hey Zen! Yes, std::array was used as the base for the class mostly because the backend data was inlined with the class (better locality). But you can use a std::vector if you want as long as performance says sharp.

BTW, there' s a test: https://github.com/sparcians/map/blame/master/sparta/test/Register/reg_bit_test.cpp that you can build by hand to test it. I forgot to hook it up to regression.

This reverts commit a954891.

furuame · 2024-08-30T14:53:56Z

BTW, there' s a test: https://github.com/sparcians/map/blame/master/sparta/test/Register/reg_bit_test.cpp that you can build by hand to test it. I forgot to hook it up to regression.

Will do!

Yes, std::array was used as the base for the class mostly because the backend data was inlined with the class (better locality). But you can use a std::vector if you want as long as performance says sharp.

std::vector version was down ~2% simulation performance on generating a 50M trace, that was a shame. I came up with another way to keep std::array, but dynamically allocate memory when the size > 64B ... testing it ...

If my trick works, that might be a good idea to shrink the std::array data to 8B as generally registers are 64-bit.

knute-mips · 2024-09-03T19:59:20Z

Look forward to your trick. :)

furuame · 2024-09-04T10:36:30Z

Done! The new version was measured 6.8% faster than the master build on generating a 10M+50M instruction trace 🚀

Register tests are also passed, which are also a good coverage on testing >64-bit RegisterBits.

klingaard

I think there's a memory leak here.

klingaard · 2024-09-04T14:45:09Z

sparta/sparta/functional/RegisterBits.hpp

-        const uint8_t         * remote_data_ = nullptr; //!< Remove data; points to local_data_ if no remote
-        const uint64_t          num_bytes_ = 0; //!< Number of bytse
+        std::array<uint8_t, 8>   local_storage_; //!< Local storage
+        std::unique_ptr<uint8_t> local_storage_alt_; //!< Alternative local storage when register size > 64B


I think you meant std::unique_ptr<uint8_t[]> here.

Oops sorry -- My “C” brain thought I could use that as a uint8_t pointer!

klingaard · 2024-09-05T13:44:16Z

My “C” brain thought I could use that as a uint8_t pointer!

I thought I trained that out of you! 🤣

klingaard · 2024-09-05T13:44:45Z

I'll take a look at the regressions to see why it's failing. It's another irritant, I think.

furuame · 2024-09-05T13:49:19Z

I'll take a look at the regressions to see why it's failing. It's another irritant, I think.

Huh it’s like on me. Weird -- I passed it on my Apple Silicon box.

 58/101 Test  #58: Register_test ....................Subprocess aborted***Exception:   0.10 sec

I remember I saw a different CI failure previously though. I’ll also take a look at this one tomorrow.

klingaard · 2024-09-05T14:09:50Z

I ran valgrind --vgdb=full --vgdb-error=0 ./Register_test and it found a few issues with RegisterBits. Might start there.

furuame · 2024-09-05T14:14:57Z

Thank you! Will do!

furuame · 2024-09-06T02:32:05Z

I ran valgrind --vgdb=full --vgdb-error=0 ./Register_test and it found a few issues with RegisterBits. Might start there.

Done! That one was tricky ... Valgrind rocks!!

Regressions still fail but that seems a Cython compilation issue on Argos ... @klingaard would you help take a look at it?

furuame · 2024-09-06T02:32:49Z

sparta/sparta/functional/Register.hpp

+        if(!isPowerOf2(def->bytes)) {
+            return RegisterBits(nullptr);
+        }


Would it be better to just throw a SpartaException or keep this as it is?

The write mask is initialized by Class initializer list, but the legal register size is later checked by the constructor ...

However an odd size would cause illegal access here.

I think you should throw.

Agree, that’s much better

klingaard · 2024-09-07T16:10:36Z

Regressions still fail but that seems a Cython compilation issue on Argos

Sigh... will take a look

…p into zhenw/dynamic-register-bits

klingaard · 2024-09-07T18:08:26Z

I'm going to remove helios/pipeview from the regression build. It's broken and we're re-writing it, so I don't care as much

klingaard · 2024-09-07T18:10:33Z

sparta/sparta/functional/RegisterBits.hpp

+        {
+            if(num_bytes_ <= local_storage_.size()) {
+                local_storage_ = orig.local_storage_;
+                local_data_ = orig.local_data_ == nullptr ? nullptr : local_storage_.data();


When does this happen?

Curious, when can num_bytes_ be <= local storage size and the original still have its local_data_ pointer set to nullptr?

When does this happen?

Actually I don’t know 😂 I just follow the same logics but take care of the original storage and alternative storage here.

Curious, when can num_bytes_ be <= local storage size and the original still have its local_data_ pointer set to nullptr?

There can be a scenario that RegisterBits is initialized by a raw pointer, which could be nullptr. That’s the only case I can think of.

But if it has > 0 bytes, will it ever be nullptr?

Yes, local_data_ could be nullptr if RegisterBits is initialized by a const uint8 pointer. In that case, only remote_data_ pointer is initialized. local_data_ is nullptr until the value is changed.

* Enable dynamic-sized RegisterBits * Revert "Enable dynamic-sized RegisterBits" This reverts commit a954891. * Keep 64B array but use heap when registers > 64B * Enable register bit tests * Downsize preallocated RegisterBits data space * Fixed smart pointer type * Fixed illegal access caused by illegal RegDef * Fixed uninitialized values * Remove large register from bad test * Moved CI forward; see what happens * Do not build helios anymore * Throw the exception in the initialze function --------- Co-authored-by: Knute Lingaard <[email protected]> ce6b9dc

furuame · 2024-09-09T05:10:53Z

I merged this into the map_v2 branch.

knute-mips · 2024-09-09T15:10:15Z

I haven't documented the release process fully, but in a nutshell (I will documented this officially 😄 ):

Merged changes into master
Merge changes from master into map_v2 branch
Tag map_v2 branch with the next tag
Make a release

furuame · 2024-09-09T15:14:12Z

I haven't documented the release process fully, but in a nutshell (I will documented this officially 😄 ):

Merged changes into master

Merge changes from master into map_v2 branch

Tag map_v2 branch with the next tag

Make a release

I am glad I do it right so far!

Would we need a process to release a tag? I haven’t done it and filed a discussion here: #519

Enable dynamic-sized RegisterBits

a954891

furuame added the component: sparta Issue is related to sparta framework label Aug 29, 2024

furuame requested a review from klingaard August 29, 2024 09:43

furuame self-assigned this Aug 29, 2024

Revert "Enable dynamic-sized RegisterBits"

75678aa

This reverts commit a954891.

furuame added 3 commits September 3, 2024 05:48

Keep 64B array but use heap when registers > 64B

be617c5

Enable register bit tests

a7503f8

Downsize preallocated RegisterBits data space

495d377

klingaard requested changes Sep 4, 2024

View reviewed changes

Fixed smart pointer type

3d0e185

furuame added 3 commits September 5, 2024 10:08

Fixed illegal access caused by illegal RegDef

8c099b3

Fixed uninitialized values

f8c9b00

Remove large register from bad test

bd372b0

furuame commented Sep 6, 2024

View reviewed changes

klingaard added 3 commits September 7, 2024 11:33

Moved CI forward; see what happens

cbd5363

Merge branch 'zhenw/dynamic-register-bits' of github.com:sparcians/ma…

a98f272

…p into zhenw/dynamic-register-bits

Do not build helios anymore

64ff279

klingaard reviewed Sep 7, 2024

View reviewed changes

klingaard approved these changes Sep 7, 2024

View reviewed changes

Throw the exception in the initialze function

233cc95

furuame merged commit ce6b9dc into master Sep 9, 2024
8 checks passed

furuame deleted the zhenw/dynamic-register-bits branch September 9, 2024 04:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable dynamic-sized RegisterBits #515

Enable dynamic-sized RegisterBits #515

furuame commented Aug 29, 2024

klingaard commented Aug 29, 2024

furuame commented Aug 30, 2024

knute-mips commented Sep 3, 2024

furuame commented Sep 4, 2024 •

edited

Loading

klingaard left a comment

klingaard Sep 4, 2024

furuame Sep 5, 2024

klingaard commented Sep 5, 2024

klingaard commented Sep 5, 2024

furuame commented Sep 5, 2024

klingaard commented Sep 5, 2024

furuame commented Sep 5, 2024

furuame commented Sep 6, 2024

furuame Sep 6, 2024 •

edited

Loading

furuame Sep 6, 2024

klingaard Sep 7, 2024

furuame Sep 9, 2024

klingaard commented Sep 7, 2024

klingaard commented Sep 7, 2024

klingaard Sep 7, 2024

klingaard Sep 7, 2024

furuame Sep 9, 2024 •

edited

Loading

knute-mips Sep 9, 2024

furuame Sep 9, 2024 •

edited

Loading

furuame commented Sep 9, 2024

knute-mips commented Sep 9, 2024

furuame commented Sep 9, 2024

Enable dynamic-sized RegisterBits #515

Enable dynamic-sized RegisterBits #515

Conversation

furuame commented Aug 29, 2024

klingaard commented Aug 29, 2024

furuame commented Aug 30, 2024

knute-mips commented Sep 3, 2024

furuame commented Sep 4, 2024 • edited Loading

klingaard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klingaard commented Sep 5, 2024

klingaard commented Sep 5, 2024

furuame commented Sep 5, 2024

klingaard commented Sep 5, 2024

furuame commented Sep 5, 2024

furuame commented Sep 6, 2024

furuame Sep 6, 2024 • 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

klingaard commented Sep 7, 2024

klingaard commented Sep 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furuame Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furuame Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

furuame commented Sep 9, 2024

knute-mips commented Sep 9, 2024

furuame commented Sep 9, 2024

furuame commented Sep 4, 2024 •

edited

Loading

furuame Sep 6, 2024 •

edited

Loading

furuame Sep 9, 2024 •

edited

Loading

furuame Sep 9, 2024 •

edited

Loading