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

[Seq] Add a preset value to allow fir registers to be preset #5667

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

nandor
Copy link
Contributor

@nandor nandor commented Jul 24, 2023

See RationaleSeq.md

lib/Dialect/Seq/SeqOps.cpp Show resolved Hide resolved
Comment on lines 164 to 168
Additionally, `sv` operations are also included to provide the register with
a randomized preset value.
a randomized preset value, unless an explicit preset is specified.
Since items assigned in an `always_ff` block cannot be initialised in an
`initial` block, this register lowers to `always`.
Copy link
Member

Choose a reason for hiding this comment

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

A slightly different formulation has the preset subsume the randomized preset value. I.e., there is a preset <value | "random"> and the presence of the "random" flavor is what produces the existing lowering. If no preset is specified, then this is equivalent to disabling randomization.

The main benefit of this is that it seems odd to lump eliding presets under disabling randomization and this provides a way out of it.

Copy link
Contributor Author

@nandor nandor Jul 24, 2023

Choose a reason for hiding this comment

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

I agree, we should introduce "preset random", but that requires a lot of test adjusments and it slightly exceeds the scope of what I need to do now. I adjusted the docs a bit to better reflect that.

lib/Dialect/Seq/Transforms/LowerSeqToSV.cpp Outdated Show resolved Hide resolved
Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

Async regreset is currently conditionally assigned to a reset value in the initial block so what's the expected behavior of async reset value with preset?

@nandor
Copy link
Contributor Author

nandor commented Jul 24, 2023

The preset assignment precedes the async reset logic, so nothing should change in that case.

@seldridge
Copy link
Member

The preset assignment precedes the async reset logic, so nothing should change in that case.

This makes sense to me. Does the reasoning below make sense?

We should separate out several things.

  1. Asynchronous reset value.
  2. Asynchronous reset value when reset asserted at time zero.
  3. Preset value.

In the IR modeling, these should be kept as distinct things. When targeting FPGA flows, there should be an optional optimization pass that moves asynchronous reset initial values to preset values if the preset value is unset or random.

I think the emission should eventually allow composition of all of these to give you either:

reg foo;
// Asynchronous reset emission w/ random preset:
initial begin
  `ifdef RANDOM_REG
  foo = `RANDOM;
  `endif
  `ifndef SYNTHESIS
  if (reset)
    foo = foo_init_expr;
  `endif
end
always @(posedge clock or posedge reset) begin
  if (reset)
    foo = foo_init_expr;
  else
    /* ... */
end
reg foo;
// Asynchronous reset emission w/ constant preset:
initial begin
  foo = foo_preset_literal;
  `ifndef SYNTHESIS
  if (reset)
    foo = foo_init_expr;
  `endif
end
always @(posedge clock or posedge reset) begin
  if (reset)
    foo = foo_init_expr;
  else
    /* ... */
end

@nandor
Copy link
Contributor Author

nandor commented Jul 24, 2023

I agree, preset is orthogonal to async reset. On FPGAs we turn async resets to sync resets so we don't have to reason about that crazyness. Unless I am mistaken, now we get what you wrote down, with the exception that we always have a preset, random or constant.

docs/Dialects/Seq/RationaleSeq.md Show resolved Hide resolved
Comment on lines +293 to +294
else if (!disableRegRandomization)
randomInit.push_back(svReg);
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, why not just emit both? The random is ifdef guarded and the preset follows the randomization and is not ifdef guarded.

This logic for emission should already work for this below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want these two to be orthogonal. If a register is preset, we don't reset it. I don't want to have to rely on macros here or SV assigment order and madness.

Copy link
Member

Choose a reason for hiding this comment

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

You've convinced me. 😉 Emitting both is just going to be a useless procedural assignment.

@nandor nandor merged commit 59fda03 into main Jul 25, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants