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

[ExportVerilog] Wires are declared after they are referenced #438

Closed
teqdruid opened this issue Jan 12, 2021 · 6 comments
Closed

[ExportVerilog] Wires are declared after they are referenced #438

teqdruid opened this issue Jan 12, 2021 · 6 comments
Labels
bug Something isn't working ExportVerilog

Comments

@teqdruid
Copy link
Contributor

In the situation where an expression cannot be inlined and appears in the IR after its use, ExportVerilog will emit the use before the declaration. Generally speaking, this will likely occur when a cycle occurs.

Setting emitInlineWireDecls (here) to false mitigates the issue. Always putting the wire declarations at the beginning of the file is the simplest thing to do, but probably not what we want.

Should we set emitInlineWireDecls to false and treat optimal decl placement as a future cleanup/enhancement?

teqdruid added a commit to teqdruid/circt that referenced this issue Jan 12, 2021
@teqdruid
Copy link
Contributor Author

Will post a test case tomorrow.

@lattner
Copy link
Collaborator

lattner commented Jan 13, 2021

Thx, I appreciate it. I am not surprised that 'cyclic' graphs aren't handled properly yet but it should be straight-forward to fix. Thanks!

@teqdruid
Copy link
Contributor Author

  rtl.module @shl(%a: i1) -> (i1 {rtl.name = "b"}) {
    %1 = rtl.add %0, %0 : i1
    %2 = rtl.sub %0, %1 : i1
    %0 = rtl.shl %a, %a : i1
    rtl.output %1 : i1
  }

emits

module shl(
  input  a,
  output b);

  wire _T = _T_0 + _T_0;        // /home/jodemme/circt/integration_test/EmitVerilog/lint.mlir:33:10
  wire _T_0 = a << a;   // /home/jodemme/circt/integration_test/EmitVerilog/lint.mlir:35:10
  assign b = _T;        // /home/jodemme/circt/integration_test/EmitVerilog/lint.mlir:36:5
endmodule

And while Verilator accepts it, Questa barfs:

-- Compiling module shl
** Error: lint.sv(49): (vlog-2730) Undefined variable: '_T_0'.
** Error (suppressible): lint.sv(50): (vlog-2388) '_T_0' already declared in this scope (shl) at lint.sv(49).

@teqdruid
Copy link
Contributor Author

I was able to repro it without a cycle.

teqdruid added a commit that referenced this issue Jan 17, 2021
* Gasket lower passes. The actual op emission yet to go.

* Capnp message decode

* Removing changes to RTL dialect

* ExtractOp -> ArraySliceOp

* Squashed commit of the following:

commit b4f85eb
Author: John Demme <[email protected]>
Date:   Tue Jan 5 11:51:51 2021 -0800

    Adding support for structs

commit 2692cb8
Merge: 5ff0abc b5240fa
Author: John Demme <[email protected]>
Date:   Tue Jan 5 11:16:00 2021 -0800

    Merge branch 'main' into rtl-reinterpret-cast

commit 5ff0abc
Author: John Demme <[email protected]>
Date:   Mon Jan 4 23:42:22 2021 -0800

    No valid cast syntax which I could find

commit 328941d
Merge: 807fd58 e6a4b00
Author: John Demme <[email protected]>
Date:   Mon Jan 4 22:31:56 2021 -0800

    Merge branch 'main' into rtl-reinterpret-cast

commit 807fd58
Merge: 690fe48 463832b
Author: John Demme <[email protected]>
Date:   Mon Jan 4 22:11:36 2021 -0800

    Merge remote-tracking branch 'upstream/main' into rtl-reinterpret-cast

commit 690fe48
Merge: fc0cddd a76e77d
Author: John Demme <[email protected]>
Date:   Mon Jan 4 22:00:05 2021 -0800

    Merge branch 'main' into rtl-reinterpret-cast

commit fc0cddd
Author: John Demme <[email protected]>
Date:   Mon Jan 4 18:58:10 2021 -0800

    ExportVerilog support

commit f361155
Author: John Demme <[email protected]>
Date:   Mon Jan 4 17:40:28 2021 -0800

    Cast(To|From)Bits

* Progress

* ExportVerilog didn't support arrays on module ports

* Revert "Squashed commit of the following:"

This reverts commit ff35a6d.

* Squashed commit of the following:

commit 6641625
Author: John Demme <[email protected]>
Date:   Mon Jan 11 16:31:36 2021 -0800

    Specify bit layout of types

commit cd924d5
Author: John Demme <[email protected]>
Date:   Mon Jan 11 15:14:35 2021 -0800

    Fixing test

commit e45b6a1
Merge: f69c496 ce05aca
Author: John Demme <[email protected]>
Date:   Mon Jan 11 15:11:03 2021 -0800

    Merge branch 'main' into rtl-reinterpret-cast

commit f69c496
Merge: eeb7dd4 ebaa869
Author: John Demme <[email protected]>
Date:   Mon Jan 11 15:10:45 2021 -0800

    Merge branch 'main' into rtl-reinterpret-cast

commit eeb7dd4
Merge: f5ee0f5 8f69a62
Author: John Demme <[email protected]>
Date:   Mon Jan 11 11:15:20 2021 -0800

    Merge remote-tracking branch 'upstream/main' into rtl-reinterpret-cast

commit f5ee0f5
Author: John Demme <[email protected]>
Date:   Sat Jan 9 19:43:49 2021 -0800

    Changing back unrelated, spurious comment formatting

commit 5abea84
Merge: b0b5cfa ad023e8
Author: John Demme <[email protected]>
Date:   Sat Jan 9 18:16:58 2021 -0800

    Merge branch 'main' into rtl-reinterpret-cast

commit b0b5cfa
Author: John Demme <[email protected]>
Date:   Sat Jan 9 18:05:41 2021 -0800

    I think this is everything except for the documentation.

commit b4f85eb
Author: John Demme <[email protected]>
Date:   Tue Jan 5 11:51:51 2021 -0800

    Adding support for structs

commit 2692cb8
Merge: 5ff0abc b5240fa
Author: John Demme <[email protected]>
Date:   Tue Jan 5 11:16:00 2021 -0800

    Merge branch 'main' into rtl-reinterpret-cast

commit 5ff0abc
Author: John Demme <[email protected]>
Date:   Mon Jan 4 23:42:22 2021 -0800

    No valid cast syntax which I could find

commit 328941d
Merge: 807fd58 e6a4b00
Author: John Demme <[email protected]>
Date:   Mon Jan 4 22:31:56 2021 -0800

    Merge branch 'main' into rtl-reinterpret-cast

commit 807fd58
Merge: 690fe48 463832b
Author: John Demme <[email protected]>
Date:   Mon Jan 4 22:11:36 2021 -0800

    Merge remote-tracking branch 'upstream/main' into rtl-reinterpret-cast

commit 690fe48
Merge: fc0cddd a76e77d
Author: John Demme <[email protected]>
Date:   Mon Jan 4 22:00:05 2021 -0800

    Merge branch 'main' into rtl-reinterpret-cast

commit fc0cddd
Author: John Demme <[email protected]>
Date:   Mon Jan 4 18:58:10 2021 -0800

    ExportVerilog support

commit f361155
Author: John Demme <[email protected]>
Date:   Mon Jan 4 17:40:28 2021 -0800

    Cast(To|From)Bits

* Decoder compiling

* Decode module working

* Decode cleanup

* Encoder done!

* Mitigation for #438

* Adding some names and adding a test

* Making the test a bit more flexible

* Remove rationale update

* Cleanup and non-capnp build fixes

* Clang-format

* Feedback from Mike
@lattner
Copy link
Collaborator

lattner commented Feb 14, 2021

I was able to repro it without a cycle.

Can you share a small testcase? I assume this is just when the operations are out of order?

lattner added a commit that referenced this issue Feb 14, 2021
This emits out of line declations in the narrow case where we need
to avoid verilog use-before-def issues.
@lattner
Copy link
Collaborator

lattner commented Feb 14, 2021

Fixed in 20eefde

@lattner lattner closed this as completed Feb 14, 2021
@darthscsi darthscsi added the bug Something isn't working label Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ExportVerilog
Projects
None yet
Development

No branches or pull requests

3 participants