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

dialects: (hw) Add InnerSymAttr attributes to HW dialect. #2251

Merged
merged 4 commits into from
Mar 7, 2024

Conversation

lucjaulmes
Copy link
Collaborator

@lucjaulmes lucjaulmes commented Feb 28, 2024

Another hopefully manageable chunk of HW, which introduces the symbol attributes. These are used in many circt dialects (e.g., seq) to define the names then referenced by the InnerRefAttrs and the like.

This is the bit of the symbol rationale that gives details.

This PR also includes a small fix on the tests, simplifying the test file and undoing the over-correction of camel case to snake case on symbol mnemonics (we want those to be compatible with CIRCT).

Thanks a lot to @PapyChacal for all the help in deciphering where CIRCT hides the symbol attr tests.

Copy link

codecov bot commented Feb 28, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 89.52%. Comparing base (720ab7a) to head (4694ff0).

Files Patch % Lines
xdsl/dialects/hw.py 93.18% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2251      +/-   ##
==========================================
+ Coverage   89.50%   89.52%   +0.01%     
==========================================
  Files         328      328              
  Lines       39327    39428     +101     
  Branches     5825     5846      +21     
==========================================
+ Hits        35201    35297      +96     
- Misses       3320     3323       +3     
- Partials      806      808       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lucjaulmes
Copy link
Collaborator Author

  • Regarding code formatting, the ... on the same line is auto-corrected that way by ruff locally. Can revert but not sure why this is happening? I just made sure I have the correct versions (running python 3.11 though), and am rebased on latest main.
  • Regarding coverage, will complete soon. Unfortunately local computing of coverage is off too.

@lucjaulmes lucjaulmes changed the title Add InnerSymAttr attributes to HW dialect. dialects: (hw) Add InnerSymAttr attributes to HW dialect. Feb 28, 2024
@lucjaulmes lucjaulmes added the dialects Changes on the dialects label Feb 28, 2024
@lucjaulmes lucjaulmes changed the title dialects: (hw) Add InnerSymAttr attributes to HW dialect. [WIP] dialects: (hw) Add InnerSymAttr attributes to HW dialect. Feb 28, 2024
@lucjaulmes lucjaulmes marked this pull request as draft February 28, 2024 19:56
@PapyChacal
Copy link
Collaborator

  • Regarding code formatting, the ... on the same line is auto-corrected that way by ruff locally. Can revert but not sure why this is happening? I just made sure I have the correct versions (running python 3.11 though), and am rebased on latest main.

    • Regarding coverage, will complete soon. Unfortunately local computing of coverage is off too.

Did you run make in the repository? AFAIU this installs dependencies as expected, i.e. the right versions of toolings.
That said, I recently had a lot of those ... reformatting show up recently, but never had a problem with them. did they trigger CI errors in this case?

@lucjaulmes
Copy link
Collaborator Author

lucjaulmes commented Feb 29, 2024

Yes CI failed on e9a0829. I updated from requirements.txt in case there were any chances, but could try a full delete and reinstall, see if that changes anything…

Edit: did that, to no avail. It seems the only difference remaining I can see is python 3.10 vs 3.11

PapyChacal added a commit that referenced this pull request Feb 29, 2024
- #2248 

Was merged a bit fast; I realized meanwhile that not all opaque syntax
attributes have this spacing 🥲 Best example in:
- #2251

With the attribute `#hw<innerSym@sym>` 🥲 

So I'm introducing `SpacedOpaqueSyntaxAttribute` for this common
behaviour, but we need attributes to be able to use opaque syntax
without hardcoding this spacing.
@lucjaulmes
Copy link
Collaborator Author

Updated on latest main & added missing bits.

@lucjaulmes lucjaulmes marked this pull request as ready for review February 29, 2024 16:32
@lucjaulmes lucjaulmes changed the title [WIP] dialects: (hw) Add InnerSymAttr attributes to HW dialect. dialects: (hw) Add InnerSymAttr attributes to HW dialect. Feb 29, 2024
@lucjaulmes lucjaulmes merged commit 520ab4a into xdslproject:main Mar 7, 2024
10 checks passed
@lucjaulmes lucjaulmes deleted the seq-hw-4 branch March 7, 2024 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialects Changes on the dialects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants