Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configurator announcement prep #64

Closed
4 tasks done
stevehoover opened this issue May 13, 2021 · 57 comments
Closed
4 tasks done

Configurator announcement prep #64

stevehoover opened this issue May 13, 2021 · 57 comments
Assignees

Comments

@stevehoover
Copy link
Owner

stevehoover commented May 13, 2021

  • The 'Download Verilog' and "Open in Makerchip IDE" buttons don't work.

  • Any pipeline staging modification can cause an error message about BUBBLEs and a simulation segfault in Makerchip. There is an ordering requirement on redirects, similar to the ordering requirement on pipeline stages. This is affected by the EXTRA_*_BUBBLEs. We could:

    • enhance WARP-V to eliminate this constraint
    • add some complexity to the configurator
    • remove the options
    • change the defaults and add some warning text in the configurator

    Let's do the 4th option. So use all 0 defaults, and add a note "EXTRA_*_BUBBLEs (0 or 1). Set to 1 to add a cycle to replay conditions to relax circuit timing. (Not all configurations are valid.)"

  • In the downloaded Verilog file the ``include "sp_default.vh"` line is still present.

  • LD_RETURN_ALIGN is different. It must be >= (EXECUTE_STAGE - NEXT_PC_STAGE) (I think).

@stevehoover
Copy link
Owner Author

stevehoover commented May 13, 2021

  • Also, please use one of the logo2.svg/png files in place of "Redwood EDA" in the footer, sized/positioned appropriately.

@stevehoover
Copy link
Owner Author

stevehoover commented May 14, 2021

  • Please add to the generated config file:
\m4_TLV_version 1d: tl-x.org
\SV
/*
Copyright <YEAR> Redwood EDA

Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/
  • Fill in the year based on the current date.

@stevehoover
Copy link
Owner Author

stevehoover commented May 14, 2021

  • "Custom" -> "Custom Pipeline" (just to make the connection with the "Pipeline" tab)

adamint added a commit that referenced this issue May 14, 2021
@stevehoover
Copy link
Owner Author

stevehoover commented May 17, 2021

A few more:

  • Please switch from:
m4+module_def
\TLV
   m4+warpv_makerchip_cnt10_tb()
   m4+warpv()
   m4+makerchip_pass_fail()
\SV
   endmodule

to

m4+module_def
\TLV
   m4+warpv_top()
\SV
   endmodule
  • Please change back to M4_NUM_CORES. My bad.
  • I had sent a sandbox w/ improved formatting. Use that.

adamint added a commit that referenced this issue May 17, 2021
@stevehoover
Copy link
Owner Author

stevehoover commented May 17, 2021

  • Under "Verilog" tab, also add --noline to the list of args, and select it by default. Descrption: "Disable `line directive in SV output.".
  • Visual indication of which version of the model is selected. Maybe shading on the rounded rectangle.

@stevehoover
Copy link
Owner Author

stevehoover commented May 17, 2021

  • There no file to open/download when the configuration GUI is the selected type. Let's just eliminate that as a selection, and make the config file the default.

@stevehoover
Copy link
Owner Author

stevehoover commented May 17, 2021

  • Let's add this to Verilog flags as well: --fmtNoSource: Do not generate \source tags for correlating pre- and post-M4 code., and enable by default.

@stevehoover
Copy link
Owner Author

stevehoover commented May 17, 2021

  • It looks like EXTRA_NON_PIPELINED_BUBBLE and EXTRA_TRAP_BUBBLE should default to 1.
  • At one point I had changed the default of EXT_M and EXT_E to 0. It looks like they are not defined if they are 1, and are therefore always 0 now. It's reasonable to always define all EXT_*.

@stevehoover
Copy link
Owner Author

stevehoover commented May 18, 2021

This one is popping up again:

  • Include statements must be modified:
    `include "./sv_url_inc/picorv32_pcpi_div.sv"  // From: "https://raw.githubusercontent.com/stevehoover/warp-v_includes/master/divmul/picorv32_pcpi_div.sv"
    
    Must become:
    m4_sv_include_url(['https://raw.githubusercontent.com/stevehoover/warp-v_includes/master/divmul/picorv32_pcpi_div.sv'])
    
    Specifically, this should be applied to the .tlv and .sv files when opened in Makerchip.
  • Also, we'll have to change the \TLV_version [\source test.tlv] 1d: tl-x.org line of the .tlv file to \m4_TLV_version ...

You can test these with EXT_F == 1.

@stevehoover
Copy link
Owner Author

stevehoover commented May 18, 2021

  • For EXT_F == 1, we need to add this above the m4_include_lib line:
    /* verilator lint_off WIDTH */
    
  • For EXT_M == 1,
    /* verilator lint_off WIDTH */
    /* verilator lint_off CASEINCOMPLETE */
    
  • EXT_B needs:
    /* verilator lint_off WIDTH */
    /* verilator lint_off PINMISSING */
    /* verilator lint_off SELRANGE */
    

(And when multiple are selected, the superset of waivers is needed.)

@adamint
Copy link
Collaborator

adamint commented May 20, 2021

Does this mean EXT_E and EXT_M are always 0, or that if they are 1 they should not be included in the TL-V config

@stevehoover
Copy link
Owner Author

I'm suggesting to always include them in the TLV. They default (now) to 0 and could be omitted, but, I'd say, always include them explicitly.

@stevehoover
Copy link
Owner Author

stevehoover commented May 20, 2021

Notice that I had unchecked "LD_RETURN_ALIGN is different. It must be >= (EXECUTE_STAGE - NEXT_PC_STAGE)" above.

  • Let's use the default test program unless the user chooses to customize it. This way we will get a different test program depending on the selected options by default.

@stevehoover
Copy link
Owner Author

stevehoover commented May 20, 2021

  • You can remove:
// -------------------------------------------------------
// Here you can provide your own assembly program that
// will be hardcoded into the instruction memory of your core.
// The syntax roughly mimics that defined by the RISC-V ISA,
// but not exactly.
// -------------------------------------------------------

(since you have it elsewhere)

@stevehoover
Copy link
Owner Author

stevehoover commented May 20, 2021

  • For most of the "Verilog" flags, rather than specifying them on the command line, you can specify them in the source file, e.g.:
\m4_TLV_version 1d --fmtXXX --fmtYYY: tl-x.org

This way, when the source or .tlv is opened in Makerchip, the args will take effect.

I think the only arg that must be on the command line is --fmtNoSource.

@adamint
Copy link
Collaborator

adamint commented May 22, 2021

Ready to test again!

@stevehoover
Copy link
Owner Author

stevehoover commented May 22, 2021

  • For MIPS, we should disable:
    • the use of a custom test program,
    • all ISA extensions. (There is a comment "RISC-V only", but I see that these report a fatal error if selected.)
    • multiple cores.
  • Use m4+warpv_top() always, not just for custom test program.
  • I've unchecked "Include statements must be modified" item above, as it doesn't seem to be working.

@adamint
Copy link
Collaborator

adamint commented May 24, 2021

Include statements were modified on open in Makerchip, but that was not displayed in the file box. Fixed!

@adamint
Copy link
Collaborator

adamint commented May 24, 2021

Made updates

@stevehoover
Copy link
Owner Author

Did you check include statements by opening TLV file (after M4) w/ F-type in Makerchip? That was the problematic case. I can't test it at the moment.

@adamint
Copy link
Collaborator

adamint commented May 27, 2021

@stevehoover this is a success, right? https://makerchip.com/sandbox/0v2fWhogD/076hzqz#
Ready for review

@stevehoover
Copy link
Owner Author

This is a failure, but not a configurator issue. WARP-V is built for SystemVerilog. It looks like you probably checked to build as Verilog. So this is a WARP-V issue. We could remove the Verilog option for now, but let's just leave it and accept the failure.

@adamint
Copy link
Collaborator

adamint commented May 27, 2021

Ok! Are there any other issues that you found?

@stevehoover
Copy link
Owner Author

stevehoover commented May 28, 2021

  • Don't touch this one:
    m4_sv_include_url(['sandpiper_gen.vh']) // Originally: `include "sandpiper_gen.vh"
    
    I think this is from the header file and probably has a different comment than the others.
  • Look at the include of picorv32_pcpi_fast_mul in this sandbox: https://makerchip.com/sandbox/0v2fWhogD/0GZhBgM#. in both the Editor and Nav-TLV.

@adamint
Copy link
Collaborator

adamint commented May 31, 2021

sandpiper_gen.vh isn't touched anymore! What do you mean by the second checkbox?

@stevehoover
Copy link
Owner Author

stevehoover commented Jun 1, 2021

  • Create and add a WARP-V favicon.

@stevehoover
Copy link
Owner Author

stevehoover commented Jun 1, 2021

  • While debugging failing configs, I've discovered a bug in the --fmtStripUniquifiers flag, so please remove that one for now. (Comment it out in your code with a TODO to add it back when mono Issue #433 is closed.

@adamint
Copy link
Collaborator

adamint commented Jun 4, 2021

warp-v.org is updated again

@stevehoover
Copy link
Owner Author

stevehoover commented Jun 4, 2021

Looking pretty solid!

  • If you enable a custom program, then select MIPS, then RISC-V again, the custom program is not editable until you uncheck and re-check "Enable custom program".
  • "Verilog" and "Program" overflow their box. A few options:
    • Expand the space.
    • Add scrolling or wrapping.
    • (For now) remove "Memory" and "I/O". They are nice to have just to show that we are not ignoring the fact that we don't address them, but they don't actually do anything.
    • Merge "Hazards" back into "Pipeline" (with a "Hazards" heading).

Thoughts? I'd say maybe merge "Hazards" and extend the size as needed.

@stevehoover
Copy link
Owner Author

stevehoover commented Jun 5, 2021

  • If no command-line args are selected, there is a space after 1d and before the : that results in a parsing error.

@stevehoover
Copy link
Owner Author

stevehoover commented Jun 5, 2021

  • Let's add an option for the warp-v version as a full URL. Also have buttons to fill in the URL field with the latest version (master) or the latest known-good version (<sha>) (and this will be the default). This will help to keep the configurator stable until we have regression testing, and it will be useful for manual testing of master. For lack of a better place, maybe put this under "Verilog". This URL would be used for: m4_include_lib(['https://raw.githubusercontent.com/stevehoover/warp-v/master/warp-v.tlv']).

@adamint
Copy link
Collaborator

adamint commented Jun 5, 2021

I'm going to extend the table size first, let me know if it looks too weird.

@adamint
Copy link
Collaborator

adamint commented Jun 5, 2021

In the future, it would be helpful to debounce inputs. I'm trialing it with the warp-v version input, and it's working very well

@adamint
Copy link
Collaborator

adamint commented Jun 5, 2021

@stevehoover addressed all requests

@stevehoover
Copy link
Owner Author

stevehoover commented Jun 5, 2021

In the future, it would be helpful to debounce inputs. I'm trialing it with the warp-v version input, and it's working very well

I don't follow. You're referring to the WARP-V version? Yeah, looks great! Though,

  • "Override" could be deleted, and maybe "tested" rather than "supported"? edit: Oh, and let's say "latest" rather than "master", since master is the whole branch, not a version.
  • The configuration panel is okay, but it's a little wonky. It follows a different sizing policy than the other components. Ideally it should be consistent with "Get your code" and "Core details". (You can remove the ":" from "Get your code:" for consistency.) And the container size adjusts to the window, but the headings overflow. Not the end of the world, but think about any easy fixes.

@stevehoover
Copy link
Owner Author

stevehoover commented Jun 5, 2021

  • It looks like when "Do not generate \source tags ..." is deselected, a source tag exists in the 1st line of the TLV file, e.g. \TLV_version [\source test.tlv] 1d. I'm not sure why, but this seems to fail in Makerchip. It is safe to remove [\source test.tlv]. Ah... this happened because we modify \TLV_version to \m4_TLV_version to be able to utilize M4 for file imports. So, yeah, delete the source tag, too, if present.

@stevehoover
Copy link
Owner Author

stevehoover commented Jun 6, 2021

I've tracked down a Verilator bug. It surfaces for the --fmtPackAll option. We can work around it with /* verilator lint_on WIDTH */. So:

  • Add /* verilator lint_on WIDTH */ for --fmtPackAll, and comment the code with: // TODO: Disabling WIDTH to work around what we think is https://github.com/verilator/verilator/issues/1613`. This can be removed once the bug is resolved and deployed in Makerchip.`

@adamint
Copy link
Collaborator

adamint commented Jun 7, 2021

updated!

@stevehoover
Copy link
Owner Author

TLV parsing is picky. You'll need to remove the extra space when removing [\source test.tlv]. Unchecked above.

@stevehoover
Copy link
Owner Author

stevehoover commented Jun 10, 2021

  • We'll need an approach to address the pop-up blocker. Maybe if we host as warp-v.makerchip.com we can avoid it?? Is there a way to detect it, and notify??

@adamint
Copy link
Collaborator

adamint commented Jun 12, 2021

What do you mean, pop-ups? I don't think I've ever gotten a pop-up notification

@adamint
Copy link
Collaborator

adamint commented Jun 12, 2021

It's possible that instead of opening a new tab we could just redirect the user in the current window, but I'm not sure that would work either

@stevehoover
Copy link
Owner Author

I think I had to unblock pop-ups the first time and forgot about it thereafter. Maybe you did the same? I was showing to someone else on their system over zoom and noticed it was blocked from opening a new tab.

@stevehoover
Copy link
Owner Author

BTW, I'm trying to resolve failures for Verilog output prior to announcing. And we should have some snazzy visualization soon. The Verilog issue is a bit gnarly and failing regression (due to an unrelated package version issue.)

@adamint
Copy link
Collaborator

adamint commented Jun 12, 2021 via email

@adamint
Copy link
Collaborator

adamint commented Jun 12, 2021

  • mobile usability

@adamint
Copy link
Collaborator

adamint commented Jun 12, 2021

Definitely a WIP for mobile, but check it out and see how you like the changes so far. Also please check if pop-ups are still there..

@stevehoover
Copy link
Owner Author

:(. Yep, Firefox blocked me.

@stevehoover
Copy link
Owner Author

stevehoover commented Jun 14, 2021

With all this talk of Intel acquiring Si-Five, it's a great time to announce the configurator.

  • Something went askew where the 2 core diagrams are stacked rather than side-by-side.

@stevehoover
Copy link
Owner Author

stevehoover commented Jun 14, 2021

  • We should have a way to report bugs. Let's just change "Dig deeper in the github repository" to "Dig deeper or report bugs in the github repository".

@stevehoover
Copy link
Owner Author

stevehoover commented Jun 14, 2021

Mobile looks great. Just some nits to track (nothing blocking announcement):

  • There's a "Get started" menu option that does nothing.
  • Button text overflows.

@adamint
Copy link
Collaborator

adamint commented Jun 15, 2021

I'll have to look more into shrinking font size on mobile. I might have just found a workaround for popups. Updated the site

@stevehoover
Copy link
Owner Author

stevehoover commented Jun 15, 2021

  • Update github warp-v SHA to latest.

@stevehoover
Copy link
Owner Author

Looks like we're done with this one!! If there's anything from here you want to track, file a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants