Removing cruft headers from library tests and guarding mutexes in the memory manager #711

sripathi-muralitharan · 2021-04-01T22:28:46Z

This PR removes cruft headers from the library tests and adds a macro guard around the use of mutexes.

Removes the use of sys/ioctl.h
Removes the use of sys/mman.h
Adds a ifdef around the memory manager mutex (using MMAN_MUTEX)

dpetrisko · 2021-04-01T23:20:06Z

I would separate out the header changes that are RISC-V agnostic and open a different PR for the hammerparrot platform

examples/library/test_struct_size/main.c

libraries/platforms/hammerparrot-vcs/software/include/argp.h

sripathi-muralitharan · 2021-04-02T03:15:45Z

To run upto compilation of the library files, I think we require hardware.mk, compilation.mk and link.mk. So, I thought I might as well add the other two which is basically the entire platform. hardware.mk, link.mk, execution.mk and library.mk are exact copies from the files in bigblade-vcs. Only compilation.mk is changed.

sripathi-muralitharan · 2021-04-02T03:26:57Z

examples/library/test_manycore_eva/main.c

@@ -166,7 +165,7 @@ int test_manycore_eva () {
                        tgt_epa = rand() % DRAM_EPA_SIZE; // Small, but we'll deal.

                        tgt_x = (rand() % (dim_x - 1)) + origin_x;
-                        tgt_y = hb_mc_config_get_dram_y(config);
+                        // tgt_y = hb_mc_config_get_dram_y(config);


No y coordinates required for DRAM pods?

There is, but it's handled in a different way. This is @mrutt92's least favorite test

This test suuuuuuuuuuuxxxxxxxxxx. When it fails, 99 times out of 100 the solution is to fix the test instead of any RTL or library code.

We use these now with pods

bsg_replicant/libraries/bsg_manycore_config_pod.h

Line 269 in c06120a

static inline hb_mc_idx_t

.

sripathi-muralitharan · 2021-04-02T03:27:22Z

examples/library/test_struct_size/main.c

@@ -26,7 +26,6 @@
 // SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.



Is this test superseded by test_packet?

Maybe? @mrutt92 ?

lol - whoops. forgot this exists.

in my defense, what a non-descript name?

I think we can nuke this.

Should I get rid of it then before we merge?

separate PR I think

dpetrisko · 2021-04-02T03:32:49Z

Sweet, my feedback is addressed, I’ll leave the rest to @drichmond and @mrutt92

dpetrisko · 2021-04-07T00:16:30Z

Can we remove the hammerparrot platform and get the header removals / additions in?

drichmond · 2021-04-07T00:30:56Z

Is that to us or @sripathi-muralitharan?

dpetrisko · 2021-04-07T00:38:05Z

@sripathi-muralitharan for the separation, @drichmond for the review :P

sripathi-muralitharan · 2021-04-07T00:51:13Z

Removed the platform makefiles and hardware. Only the header file additions and removals now remain.

drichmond · 2021-04-07T00:54:41Z

What about the argp header, and changes to .gitignore?

sripathi-muralitharan · 2021-04-07T00:58:48Z

argp.h is required for the software port. Its used at some point. I have pointed to the file from where I take it and its an exact copy from that location. Unfortunately, this header does not exist in the newlib+dramfs toolchain, so I had to copy it over from the linux version.

I can get rid of the .gitignore

drichmond · 2021-04-07T00:59:15Z

But that's part of the platform

sripathi-muralitharan · 2021-04-07T01:01:22Z

Wait, you don't want any of the headers at all?

drichmond · 2021-04-07T01:02:40Z

examples/library/test_manycore_alignment/main.c

@@ -164,7 +163,7 @@ int test_manycore_alignment() {
                                                 dram_coord_y,
                                                 BASE_ADDR + 1);
                        write_data = rand();
-                        err = hb_mc_manycore_write32(mc, &npa, &write_data);
+                        err = hb_mc_manycore_write32(mc, &npa, write_data);


Is this just an outdated test? What went wrong here?

The hb_mc_manycore_write* functions call the hb_mc_manycore_write functions by passing the address of the third argument (i,e write_data here). Passing the third argument as an address, would make the call to hb_mc_manycore_write, a pointer to a pointer. Is that the intention? If it is, I am wrong and I will revert it.

No, you're probably right. Just confused why this wasn't caught before. Does the test pass?

Yes, the test passes.

Mostly because I'm still miffed :p

Both versions of the test (with the changes I proposed and without them) work. So, I am not sure what is the course of action of here. Keep it the same?

Oh an always passing test, that’s a classic.

Course of action is revert the change and file an issue

Reverted change and opened #713 for this.

Also found that the test_pod_iteration library test fails due to an Invalid Input error raised by the hb_mc_platform_init function. Opened #712 for that

drichmond · 2021-04-07T01:03:52Z

git rm libraries/platforms/hammerparrot-vcs/

Nothing in that directory should be in this PR

dpetrisko · 2021-04-07T01:14:27Z

argp.h is required for the software port. Its used at some point. I have pointed to the file from where I take it and its an exact copy from that location. Unfortunately, this header does not exist in the newlib+dramfs toolchain, so I had to copy it over from the linux version.

I guess the question is if we want to use different headers for different platforms. Totally up to you, but it makes sense to me to include this header in the main library so that everything is using the same C++ codebase

drichmond · 2021-04-07T01:39:07Z

argp.h is required for the software port. Its used at some point. I have pointed to the file from where I take it and its an exact copy from that location. Unfortunately, this header does not exist in the newlib+dramfs toolchain, so I had to copy it over from the linux version.

I guess the question is if we want to use different headers for different platforms. Totally up to you, but it makes sense to me to include this header in the main library so that everything is using the same C++ codebase

If that's the case, then the main libraries directory, or the examples directory are better places. And it should be a separate PR.

dpetrisko · 2021-04-07T02:08:37Z

argp.h is required for the software port. Its used at some point. I have pointed to the file from where I take it and its an exact copy from that location. Unfortunately, this header does not exist in the newlib+dramfs toolchain, so I had to copy it over from the linux version.

I guess the question is if we want to use different headers for different platforms. Totally up to you, but it makes sense to me to include this header in the main library so that everything is using the same C++ codebase

If that's the case, then the main libraries directory, or the examples directory are better places. And it should be a separate PR.

Yup agree with both

mrutt92

PR looks good to me. I might change the name of this PR to "Removes cruft headers and macro guards mutex" and edit the description with bullets saying what it does. The current name suggests something... bigger.

taylor-bsg · 2021-04-08T13:36:57Z

I suggest having a link from the EVA code to this test as something that needs to be updated if EVA map is changed.

…

On Wed, Apr 7, 2021 at 12:45 PM mrutt92 ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In examples/library/test_manycore_eva/main.c <#711 (comment)> : > @@ -166,7 +165,7 @@ int test_manycore_eva () { tgt_epa = rand() % DRAM_EPA_SIZE; // Small, but we'll deal. tgt_x = (rand() % (dim_x - 1)) + origin_x; - tgt_y = hb_mc_config_get_dram_y(config); + // tgt_y = hb_mc_config_get_dram_y(config); This test suuuuuuuuuuuxxxxxxxxxx. When it fails, 99 times out of 100 the solution is to fix the test instead of any RTL or library code. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#711 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEFG5AEXWTOG3E47J276NV3THSY4XANCNFSM42H4IWOQ> .

dpetrisko · 2021-04-20T22:03:07Z

This ready to go? Possibly with new issue opening for this:

I suggest having a link from the EVA code to this test as something that
needs to be updated if EVA map is changed.

sripathi-muralitharan · 2021-04-20T22:13:45Z

New issue or just a link to https://github.com/bespoke-silicon-group/bsg_manycore/blob/ci_bigblade/v/bsg_manycore_eva_to_npa.v?
There already seems to be a link back to the cuda-lite library code in the SV file.

dpetrisko · 2021-04-20T22:38:37Z

I think michael's suggestion is to add a link to the brittle test as well as the library

drichmond · 2021-04-20T23:04:55Z

libraries/bsg_manycore_memory_manager.h

+                #ifdef _MMAN_MUTEX_
+                  std::mutex mMemManagerMutex;
+                #endif


is this a standard macro? Or is this something we need to define now?

I don't think it is a standard macro. We will need to define it separately. Is libraries.mk a good spot for this?

Is there a more basic switch? Like if newlib?

This might conflict with something in newlib. I guess, we can be safer and say if NOPARROT or something like that. What do you think?

I like the macro how it is.

Not standard (and doesn't conflict with anything): good
Descriptive (does exactly what the name would imply): good
Does the job: excellent
What's the problem?

Well, the original question was if this was a standard macro which you just confirmed it isn't. So, there is no problem.

sripathi-muralitharan · 2021-04-29T15:58:32Z

Are there any other changes required for this PR?

mrutt92 · 2021-04-30T03:09:59Z

I think this fine, let's hammer this parrot!

… memory manager (#711) * Software fixes * Add comment to update EVA tests when EVA map is changed

dpetrisko reviewed Apr 1, 2021

View reviewed changes

examples/library/test_struct_size/main.c Outdated Show resolved Hide resolved

dpetrisko reviewed Apr 1, 2021

View reviewed changes

libraries/platforms/hammerparrot-vcs/software/include/argp.h Outdated Show resolved Hide resolved

sripathi-muralitharan commented Apr 2, 2021

View reviewed changes

drichmond reviewed Apr 7, 2021

View reviewed changes

sripathi-muralitharan force-pushed the bigblade-hammerparrot branch 2 times, most recently from 876a554 to 9c3a94b Compare April 7, 2021 01:41

Software fixes

7c9f717

sripathi-muralitharan force-pushed the bigblade-hammerparrot branch from 9c3a94b to 7c9f717 Compare April 7, 2021 06:50

mrutt92 self-requested a review April 7, 2021 19:54

mrutt92 approved these changes Apr 7, 2021

View reviewed changes

sripathi-muralitharan changed the title ~~Software Port from x86 GCC to RISC-V Newlib+Dramfs~~ Removing cruft headers from library tests and guarding mutexes in the memory manager Apr 7, 2021

Add comment to update EVA tests when EVA map is changed

acd5437

drichmond reviewed Apr 20, 2021

View reviewed changes

mrutt92 merged commit 4ea650a into bespoke-silicon-group:bigblade Apr 30, 2021

mrutt92 pushed a commit that referenced this pull request May 1, 2021

Removing cruft headers from library tests and guarding mutexes in the…

557db46

… memory manager (#711) * Software fixes * Add comment to update EVA tests when EVA map is changed

dpetrisko pushed a commit that referenced this pull request Oct 4, 2021

Removing cruft headers from library tests and guarding mutexes in the…

cd8a41a

… memory manager (#711) * Software fixes * Add comment to update EVA tests when EVA map is changed

dpetrisko pushed a commit that referenced this pull request Jan 17, 2023

Removing cruft headers from library tests and guarding mutexes in the…

a35fc36

… memory manager (#711) * Software fixes * Add comment to update EVA tests when EVA map is changed

		@@ -26,7 +26,6 @@
		// SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

Removing cruft headers from library tests and guarding mutexes in the memory manager #711

Removing cruft headers from library tests and guarding mutexes in the memory manager #711

Conversation

sripathi-muralitharan commented Apr 1, 2021 • edited Loading

dpetrisko commented Apr 1, 2021

sripathi-muralitharan commented Apr 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpetrisko commented Apr 2, 2021

dpetrisko commented Apr 7, 2021

drichmond commented Apr 7, 2021

dpetrisko commented Apr 7, 2021

sripathi-muralitharan commented Apr 7, 2021

drichmond commented Apr 7, 2021

sripathi-muralitharan commented Apr 7, 2021

drichmond commented Apr 7, 2021

sripathi-muralitharan commented Apr 7, 2021

Choose a reason for hiding this comment

sripathi-muralitharan Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sripathi-muralitharan Apr 7, 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

sripathi-muralitharan Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

drichmond commented Apr 7, 2021

dpetrisko commented Apr 7, 2021

drichmond commented Apr 7, 2021

dpetrisko commented Apr 7, 2021

mrutt92 left a comment

Choose a reason for hiding this comment

taylor-bsg commented Apr 8, 2021 via email

dpetrisko commented Apr 20, 2021

sripathi-muralitharan commented Apr 20, 2021

dpetrisko commented Apr 20, 2021

Choose a reason for hiding this comment

sripathi-muralitharan Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sripathi-muralitharan Apr 29, 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

sripathi-muralitharan commented Apr 29, 2021

mrutt92 commented Apr 30, 2021

sripathi-muralitharan commented Apr 1, 2021 •

edited

Loading

sripathi-muralitharan commented Apr 2, 2021 •

edited

Loading

sripathi-muralitharan Apr 7, 2021 •

edited

Loading

sripathi-muralitharan Apr 7, 2021 •

edited

Loading

sripathi-muralitharan Apr 7, 2021 •

edited

Loading

sripathi-muralitharan Apr 20, 2021 •

edited

Loading

sripathi-muralitharan Apr 29, 2021 •

edited

Loading