Skip to content

Conversation

@Scheremo
Copy link
Contributor

@Scheremo Scheremo commented Oct 18, 2025

This patch extends ImportVerilog’s capture mechanism to include write-side
(LHS) captures, complementing the existing read-side collection. Tasks and
functions that assign to variables in an enclosing scope now have those refs
added as explicit function parameters during lowering.

  • Context API

    • Introduce variableAssignCallback: std::function<void(mlir::Operation *)>.
    • Both read and write callbacks are chained and restored via scope guards.
  • Capture collection

    • Reads: keep existing rvalueReadCallback behavior.
    • Writes: new callback fires on moore.blocking_assign,
      moore.nonblocking_assign, and moore.delayed_nonblocking_assign.
      The LHS ref (getDst()) is captured when it originates outside the
      function’s region.
  • Lowering changes

    • In Expressions.cpp, creation sites for blocking / (delayed) non-blocking
      assigns invoke variableAssignCallback.
    • In Structure.cpp::convertFunction, install both callbacks to accumulate
      captures/captureIndex for the function, covering both reads and
      writes.
    • Existing finalizeFunctionBodyCaptures retypes the function to include the
      new capture parameters and replaces internal uses with the new block args.
  • testLHSTaskCapture in basic.sv:

    • Confirms that a task assigning to a parent variable correctly captures it as
      an additional function argument.
    • Ensures call sites and function bodies are lowered with consistent capture
      semantics even when the function is defined after its use.

@Scheremo Scheremo force-pushed the pr-assign-callback branch 8 times, most recently from 7a8ba7b to ab870c1 Compare October 18, 2025 16:33
This patch extends ImportVerilog’s capture mechanism to include write-side
(LHS) captures, complementing the existing read-side collection. Tasks and
functions that assign to variables in an enclosing scope now have those refs
added as explicit function parameters during lowering.

- **Context API**
  - Introduce `variableAssignCallback: std::function<void(mlir::Operation *)>`.
  - Both read and write callbacks are chained and restored via scope guards.

- **Capture collection**
  - **Reads**: keep existing `rvalueReadCallback` behavior.
  - **Writes**: new callback fires on `moore.blocking_assign`,
    `moore.nonblocking_assign`, and `moore.delayed_nonblocking_assign`.
    The LHS ref (`getDst()`) is captured when it originates outside the
    function’s region.

- **Lowering changes**
  - In `Expressions.cpp`, creation sites for blocking / (delayed) non-blocking
    assigns invoke `variableAssignCallback`.
  - In `Structure.cpp::convertFunction`, install both callbacks to accumulate
    `captures`/`captureIndex` for the function, covering **both** reads and
    writes.
  - Existing `finalizeFunctionBodyCaptures` retypes the function to include the
    new capture parameters and replaces internal uses with the new block args.

- **`testLHSTaskCapture`** in `basic.sv`:
  - Confirms that a task assigning to a parent variable correctly captures it as
    an additional function argument.
  - Ensures call sites and function bodies are lowered with consistent capture
    semantics even when the function is defined *after* its use.
@Scheremo Scheremo marked this pull request as ready for review October 18, 2025 17:56
Copy link
Contributor

@fabianschuiki fabianschuiki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I'm wondering if one approach to dealing with function captures in the future would be to lower the functions (without using the callbacks), and to then walk the generated function IR and find all SSA values that are used but defined outside the function. And then add those to the function as arguments.

@Scheremo
Copy link
Contributor Author

LGTM! I'm wondering if one approach to dealing with function captures in the future would be to lower the functions (without using the callbacks), and to then walk the generated function IR and find all SSA values that are used but defined outside the function. And then add those to the function as arguments.

I don't see why not! 😄 What would the advantage of such an approach be?

@Scheremo Scheremo merged commit ce803aa into llvm:main Oct 20, 2025
7 checks passed
@fabianschuiki
Copy link
Contributor

I think the only benefit would be that it removes a source of bugs when we forget to call the callback in some function, or something to that end. Instead of collecting the information separately that then must match the IR (no SSA value uses from outside), that would use the IR itself to detect that.

@Scheremo Scheremo deleted the pr-assign-callback branch October 21, 2025 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants