opensocsysarch / CoreGenPortal

OpenSoC System Architect CoreGen Portal Graphical User Interface
http://www.systemarchitect.tech/
Apache License 2.0
1 stars 0 forks source link

Complete Register Edit Window Happy Path #65

Closed fconlon closed 5 years ago

fconlon commented 5 years ago

Criteria 1

Given:

When:

Then:

Criteria 2

Given:

When:

Then:

Criteria 3

Given:

When:

Then:

Criteria 4

Given:

When:

Then:

Criteria 5

Given:

When:

Then:

Criteria 6

Given:

When:

Then:

Criteria 7

Given:

When:

Then:

fconlon commented 5 years ago

@jleidel Am I remembering correctly when I remember you saying that register width is derived and I don't need to provide edit functionality for it? There is currently no functionality in the backend to set the register width.

jleidel commented 5 years ago

@jleidel Am I remembering correctly when I remember you saying that register width is derived and I don't need to provide edit functionality for it? There is currently no functionality in the backend to set the register width.

No, register widths are required using the Width keyword in the register definition within the Yaml. You're thinking of the instruction width (which is derived from the associated instruction format)

fconlon commented 5 years ago

Follow up question. I don't see a way to unset the isSIMD bool in the register class and the only way to set it to true is SetSIMD(int width). I'm not sure how to handle that on the front end. My first thought is to make it a text box instead of a checkbox and have an empty string set it to false while a non-empty string would set the width. The backend would still need a function to set the isSIMD variable to false though

jleidel commented 5 years ago

I can add an interface to unset the SIMD flag. Your frontend method seems reasonable.

jleidel commented 5 years ago

Issue referenced here: https://github.com/opensocsysarch/CoreGen/issues/188

jleidel commented 5 years ago

Functionality add here: https://github.com/opensocsysarch/CoreGen/commit/15f997b0878a4fa759db1a0173d4c93ac7ebef48

fconlon commented 5 years ago

Awesome. I'm looking at the CoreGenReg code and the PC reg attribute is not being unset in the SetAttrs() function. Is that intentional or is that a bug?

Edit: I see that it's actually unsetting the mutually exclusive parts of the Attrs int. Could it just set Attrs equal to the new Attrs? I have to run it through gdb to confirm, but I think the unsetting is causing an issue with my attempt to set the RW flag if the user unchecks RO and check RW. I'm not sure if that's the actual issue, but it won't save the flag the first time that they user clicks save, but if they reopen the window and check RW it will save it.

fconlon commented 5 years ago

I was able to hack it together by calling unset then set twice back to back, but there may or may not be a bug in the way the attrs are being set on the backend.

fconlon commented 5 years ago

@jleidel Is there a reason to return 1 when isSIMD is set to false? The function you wrote that unsets simd sets the SIMDWidth to 0 but the GetSIMDWidth() function returns 1 if isSIMD is set to false.

fconlon commented 5 years ago

@jleidel The current version of the SIMD width box will display an empty string if isSIMD is false and will unset isSIMD if 0 or an empty string is entered into the box. However the backend will always return 1 for GetSIMDWidth when isSIMD is false, regardless of the fact that SIMDWidth is set to 0. I'm going to close this issue since it is working as intended under the current version of the backend. If changes need to be made I will open another issue.

jleidel commented 5 years ago

@fconlon, that functionality is correct. GetSIMDWidth returns 1 when SIMD is disabled. EG, the register is a scalar register (width = 1). When width > 1, then the register should be a SIMD register.

fconlon commented 5 years ago

That makes sense to me and I figured the initial intent was, but it was confusing that the unset simd set the width to 0. I'll change the UI to set the width accordingly and I'll make an input of 1 unset the simd as well. I'll also populate the box with a 1 instead of an empty string when issimd is false.