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

Indefinite parsing and type-checking loops for some parameterized procs #1615

Closed
lpawelcz opened this issue Sep 19, 2024 · 2 comments
Closed
Assignees
Milestone

Comments

@lpawelcz
Copy link
Contributor

Describe the bug
While working on #1613, we encountered an issue linked to afc720d, which appears to cause indefinite parsing and type-checking loops for certain files, such as mem_writer.x (provided in this PR).

To Reproduce
Steps to reproduce the behavior:

  1. Checkout on the branch from Add procs for interfacing with AXI peripherals in ZSTD Decoder #1613
  2. Execute the command bazel build -c opt //xls/modules/zstd/memory:mem_writer_verilog
  3. Observe indefinite loop in parsing and type-checking mem_writer DSLX files:
    Converting DSLX file to XLS IR: xls/modules/zstd/memory/mem_writer.x; 48s remote-cache, processwrapper-sandbox
    Parsing and type checking DSLX source files of target mem_writer_dslx; 48s remote-cache, processwrapper-sandbox
  1. Cancel bazel build
  2. Revert afc720d
  3. Repeat the bazel build command: bazel build -c opt //xls/modules/zstd/memory:mem_writer_verilog
  4. Observe correct IR and verilog generation for the MemWriter proc.

Expected behavior
Toolchain should be able to generate IR for the provided MemWriter proc .

Additional context
This is a blocker for #1613 and as a consequence also for #1211.

@lpawelcz lpawelcz changed the title Indefinite parsing and type-checking loops for parameterized procs Indefinite parsing and type-checking loops for some parameterized procs Sep 19, 2024
@richmckeever richmckeever self-assigned this Sep 27, 2024
@richmckeever richmckeever added this to the DSLX Next milestone Sep 27, 2024
@richmckeever
Copy link
Collaborator

What's happening here is that a couple of bugs, which are inconsistent sequencing of the parametric pattern "AxiStream<DATA_W, DEST_W, ID_W, DATA_W_DIV8>" in the zstd code, are exposing parametric env leakage in type checking, where we build maps like:

DEST_W => DATA_W_DIV8
DATA_W => DATA_W
DATA_W_DIV8 => ID_W
ID_W => DEST_W

and then try to figure out what e.g. DATA_W_DIV8 is, using such a map.

The faulty "AxiStream" are in axi_writer.x and mem_writer.x. You may be able to get unblocked by fixing those to be consistent with the struct declaration. In the meantime I will work on a fix in the type checker.

@lpawelcz
Copy link
Contributor Author

lpawelcz commented Oct 1, 2024

@richmckeever thank you so much for pointing the errors in the sequencing of the parameters! I was able to fix those and generate the IR without problems.

copybara-service bot pushed a commit that referenced this issue Oct 10, 2024
In implementing the fix for #1615, we added `ResolveNominalTypeDims` function, which is used by `ResolveInternal` as a safer implementation of its use case than pushing a map of possibly extraneous dims through `AddNominalTypeDims`.

It was theorized that we then no longer needed the ability for `ConcretizeStructAnnotation` to incrementally populate dims, but this was  disproven by code in the zstd_dec_verilog target on a branch. This change restores the incremental add support, but keeps it only used by `ConretizeStructAnnotation`.

Fixes #1655

PiperOrigin-RevId: 684489422
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

3 participants
@richmckeever @lpawelcz and others