feat: hierarchical flattening #90

uduse · 2023-01-13T19:46:22Z

Hierarchical flattening for modules discussed in #86.

TODO:

remove tree dependency
add tests
add more docs

codecov · 2023-01-13T19:49:06Z

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (831e8d3) 87.36% compared to head (cd5950a) 87.30%.

❗ Current head cd5950a differs from pull request most recent head 094d6c5. Consider uploading reports for the commit 094d6c5 to get more accurate results

Files	Patch %	Lines
hdl21/flatten.py	88.88%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
- Coverage   87.36%   87.30%   -0.07%     
==========================================
  Files         107      111       +4     
  Lines        9784     9721      -63     
==========================================
- Hits         8548     8487      -61     
+ Misses       1236     1234       -2

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

dan-fritchman · 2023-01-17T18:02:02Z

Looking great.

I don't know why it won't let me leave this as a comment - but:

def is_flat(m: h.Instance | h.Instantiable) -> bool:

Does that "or operator creates Union types" work in Pythons v3.7-3.9?
I think it's only 3.10+.

dan-fritchman · 2023-01-17T18:15:45Z

OK commenting on specific lines is totally broken(?).
More on is_flat, particularly the types of things that are & aren't "flat" -

PrimitiveCall and ExternalModuleCall are indeed flat
GeneratorCall generally is not.

Each generator-call, having been through elaboration, will have a .result field, which is a Module.
This'll just need the tweak to, if encountering a GeneratorCall, inspect its result as a module.

uduse · 2023-01-18T04:09:51Z

Does that "or operator creates Union types" work in Pythons v3.7-3.9?
I think it's only 3.10+.

Oh, right. My Python version is higher than Hdl21. I will update it with the older syntax. In terms supporting types other than plain primitives, I am honestly not quite sure because I have no clue how to use them properly.

Let me add some basic tests first and gradually support more cases from there.

uduse · 2023-04-12T00:35:09Z

ping

dan-fritchman · 2023-04-12T01:38:12Z

Sorry what's the "ping" there?
I left a bunch of commentary back in January. Did it get resolved?

uduse · 2023-04-12T03:07:05Z

Sorry what's the "ping" there? I left a bunch of commentary back in January. Did it get resolved?

Sorry, just wanted to know what's the status of the PR here. The PR now implements the base use case well and do have some tests that cover such usage. I think it's a good starting point?

hdl21/flatten.py

dan-fritchman · 2023-01-17T17:57:22Z

hdl21/flatten.py


    new_module = h.Module((m.name or "module") + "_flat")
    for port_name in m.ports:
        new_module.add(h.Port(name=port_name))

+    # add all signals to the root level
    for n in nodes:
        for sig in n.conns.values():
            sig_name = sig.name
            if sig_name not in new_module.ports:
                new_module.add(h.Signal(name=sig_name))


Same comment as for Ports on line 99 - we should copy the Signal attributes (besides its name).

There's also a _copy_to_internal function which copies everything about a port, except for its porti-ness, in case we need that:

Hdl21/hdl21/signal.py

Line 107 in 945aa6e

def _copy_to_internal(sig: Signal) -> Signal:

. (I don't think we will?)

I am not exactly sure what this _copy_to_internal does and it seems in my application I don't need it at all.

hdl21/flatten.py

dan-fritchman · 2023-04-12T15:51:47Z

Yeah I think the feedback should be pretty straightforward to follow up on, and we can get that merged. Happy to add any detail if any of it's unclear.

uduse · 2023-05-25T01:18:56Z

@dan-fritchman updated based on your feedback, passes tests locally.

Mostly catch up to Vlsir main, un-block #90

dan-fritchman · 2023-05-31T22:44:01Z

hdl21/flatten.py

+def is_flat(m: Union[h.Instance, h.Instantiable]) -> bool:
+    if isinstance(m, h.Instance):
+        return is_flat(m.of)
+    elif isinstance(m, (h.PrimitiveCall, h.GeneratorCall, h.ExternalModuleCall)):


A GeneratorCall is flat?
Why not flatten its Module-result?

This function is actually just is_flat, come to think of it - so should be something like:

if isinstance(m, h.GeneratorCall): return is_flat(m.result)

hm... this decision is kind arbitrary because I don't use GeneratorCall so it becomes a case I don't handle. Any suggestion how it should be handled here? Return True if its generated module is flat?

dan-fritchman · 2023-05-31T23:30:04Z

Added a few more tests here:
04eba75

There's 3 things, two related groups:

Supporting Generators should be pretty quick. They've already been elaborated ("run"); just dispatch all the flattening-code to their result Module.
Slice and Concat will have some more mechanics. There is an existing elaborator-pass hdl21/elab/elaborators/slices.py which does "resolution" for them on a per-Module basis. I expect flattening will need something similar, enacting this across Instance/ Module boundaries.

dan-fritchman · 2023-05-31T23:41:15Z

A few more things not included in those tests which should be added, and should be pretty straightforward:

Instances of ExternalModules
Instances of Generator (calls)

uduse · 2023-11-24T01:51:56Z

Seems GeneratorCall is no longer explicit. I am taking that part out.
I'm not exactly sure how to handle Slice and Concat. How about this: there will be no Slice and Concat on the flattened module, only Signal with various widths.

dan-fritchman · 2023-05-31T22:49:27Z

hdl21/flatten.py

+        new_conns = {}
+        new_parents = parents + (inst,)
+        for src_port_name, sig in inst.conns.items():
+            if isinstance(sig, h.Signal):


Things that will be in inst.conns.values, in addition to Signal, will include: Slice, Concat.

There is a Connectable union of everything that can go into conns before elaboration. After, that should be reduced to:

Connectable = Union[ # Still gonna be there: "Signal", "Slice", "Concat", # Removed during elaboration: "NoConn", "PortRef", # And all this `Bundle` stuff is definitely removed via elaboration: "BundleInstance", "AnonymousBundle", "BundleRef", ]

dan-fritchman · 2023-05-31T22:51:31Z

hdl21/flatten.py

+def is_flat(m: Union[h.Instance, h.Instantiable]) -> bool:
+    if isinstance(m, h.Instance):
+        return is_flat(m.of)
+    elif isinstance(m, (h.PrimitiveCall, h.GeneratorCall, h.ExternalModuleCall)):


This function is actually just is_flat, come to think of it - so should be something like:

if isinstance(m, h.GeneratorCall): return is_flat(m.result)

dan-fritchman · 2023-05-31T23:37:48Z

hdl21/flatten.py

+                raise ValueError(f"signal {key} not found")
+            new_conns[src_port_name] = target_sig
+
+        if isinstance(inst.of, h.PrimitiveCall):


Needs cases for:

GeneratorCall - which will need to be flattened, and

ExternalModuleCall - which we should regard as "already flat"

dan-fritchman · 2023-12-18T19:18:49Z

OK here's a few proposed tweaks: #213.
Including (for now) just raising a NotImplementedError when encountering slices and/or concats.
Can either roll those commits in here, or use that PR instead.

uduse · 2023-12-24T06:16:06Z

Closing in favor of #213

uduse requested a review from dan-fritchman January 24, 2023 21:16

uduse self-assigned this Jan 24, 2023

uduse added the enhancement New feature or request label Jan 24, 2023

uduse linked an issue Jan 24, 2023 that may be closed by this pull request

Hierarchical Flattening for Modules #86

Open

uduse force-pushed the uduse/flatten branch from e75b1ce to 1790729 Compare January 24, 2023 22:55

uduse marked this pull request as ready for review January 24, 2023 22:56

uduse changed the title ~~feat: hierarchical flattening draft~~ feat: hierarchical flattening Jan 24, 2023

dan-fritchman requested changes Apr 12, 2023

View reviewed changes

uduse force-pushed the uduse/flatten branch from 1790729 to 09f227c Compare May 25, 2023 01:12

uduse marked this pull request as draft May 25, 2023 01:14

dan-fritchman added a commit that referenced this pull request May 25, 2023

Merge pull request #118 from dan-fritchman/whatever_dans_doing

bc27da9

Mostly catch up to Vlsir main, un-block #90

uduse force-pushed the uduse/flatten branch from afd620b to 061fc7c Compare May 25, 2023 23:27

uduse marked this pull request as ready for review May 26, 2023 19:31

uduse requested a review from dan-fritchman May 26, 2023 19:31

dan-fritchman reviewed May 31, 2023

View reviewed changes

uduse force-pushed the uduse/flatten branch from 8723673 to cd5950a Compare August 26, 2023 04:41

uduse added 3 commits November 23, 2023 17:10

feat: hierarchical flattening draft

8dc27d8

refactor: remove tree dependency and clean up logging

ad6212c

fix: type hint for older Python version

d7dbc24

uduse added 6 commits November 23, 2023 17:10

feat: update syntax and add tests

e26f14b

test: improve coverage by testing node repr

c0d28e4

feat: updates flatten.py

e55c2bf

feat: update flatten.py for docs and format test_flatten.py

07d304e

feat: update docs for flatten.py

b0df06f

fix: python 3.7 syntax

6592955

uduse force-pushed the uduse/flatten branch from cd5950a to 6592955 Compare November 23, 2023 22:11

dan-fritchman and others added 2 commits November 23, 2023 18:23

Add flattening tests for Generators, Slices, Concats

4cb4f00

remove GeneratorCall test

094d6c5

uduse requested a review from dan-fritchman November 24, 2023 01:52

uduse marked this pull request as draft November 24, 2023 01:52

dan-fritchman approved these changes Dec 18, 2023

View reviewed changes

dan-fritchman mentioned this pull request Dec 18, 2023

Flattening Tweaks #213

Merged

uduse closed this Dec 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: hierarchical flattening #90

feat: hierarchical flattening #90

uduse commented Jan 13, 2023 •

edited

Loading

codecov bot commented Jan 13, 2023 •

edited

Loading

dan-fritchman commented Jan 17, 2023

dan-fritchman commented Jan 17, 2023

uduse commented Jan 18, 2023

uduse commented Apr 12, 2023

dan-fritchman commented Apr 12, 2023

uduse commented Apr 12, 2023

dan-fritchman Jan 17, 2023

uduse May 25, 2023

dan-fritchman commented Apr 12, 2023

uduse commented May 25, 2023

dan-fritchman May 31, 2023

dan-fritchman May 31, 2023

uduse May 31, 2023

dan-fritchman commented May 31, 2023

dan-fritchman commented May 31, 2023

uduse commented Nov 24, 2023

dan-fritchman May 31, 2023

dan-fritchman May 31, 2023

dan-fritchman May 31, 2023

dan-fritchman commented Dec 18, 2023 •

edited

Loading

uduse commented Dec 24, 2023

feat: hierarchical flattening #90

feat: hierarchical flattening #90

Conversation

uduse commented Jan 13, 2023 • edited Loading

codecov bot commented Jan 13, 2023 • edited Loading

Codecov Report

dan-fritchman commented Jan 17, 2023

dan-fritchman commented Jan 17, 2023

uduse commented Jan 18, 2023

uduse commented Apr 12, 2023

dan-fritchman commented Apr 12, 2023

uduse commented Apr 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-fritchman commented Apr 12, 2023

uduse commented May 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-fritchman commented May 31, 2023

dan-fritchman commented May 31, 2023

uduse commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-fritchman commented Dec 18, 2023 • edited Loading

uduse commented Dec 24, 2023

uduse commented Jan 13, 2023 •

edited

Loading

codecov bot commented Jan 13, 2023 •

edited

Loading

dan-fritchman commented Dec 18, 2023 •

edited

Loading