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

Fix asyncRam#: multiple clocks, undefineds, laziness #2031

Merged
merged 1 commit into from Jan 7, 2022

Conversation

DigitalBrains1
Copy link
Member

This is a partial backport of #2006.

  1. In Haskell simulation, the way read samples were produced, with
    unsafeSynchronizer, was simply wrong. It would compress and duplicate
    samples while traversing clock domains, which is not a correct model of
    how it works.

The generated HDL was fine, it only affected Haskell simulation.

  1. The Haskell model of asyncRAM# treated an undefined write enable
    as an asserted enable. But an undefined value in Haskell can
    correspond to any value whatsoever in HDL, so HDL simulation might or
    might not write. With this commit, the XException of the write enable
    is written as the value in the RAM, since it could have either been
    written to or not been written to. On the next read of that address, it
    will return the XException.

This issue did not propagate to asyncRam and asyncRamPow2, since
there, the same condition also causes the write address to be undefined,
and this is properly handled by the primitive.

  1. The asyncRAM# primitive was also too strict in most of its inputs.
    Combinatorially feeding the read output to the write-side inputs would
    lock up the simulation, while it is a valid circuit. This problem did
    not propagate to the asyncRam and asyncRamPow2 functions, which are
    lazy enough because the signals go through Signal's fmap and <*>.

  2. Data written to memory is seqXd for efficiency.

Additionally, documentation for memory components was harmonized and
corrected.

Still TODO:

  • Write a changelog entry (see changelog/README.md)
  • Check copyright notices are up to date in edited files

This is a partial backport of #2006.

1. In Haskell simulation, the way read samples were produced, with
`unsafeSynchronizer`, was simply wrong. It would compress and duplicate
samples while traversing clock domains, which is not a correct model of
how it works.

The generated HDL was fine, it only affected Haskell simulation.

2. The Haskell model of `asyncRAM#` treated an _undefined_ write enable
as an asserted enable. But an _undefined_ value in Haskell can
correspond to any value whatsoever in HDL, so HDL simulation might or
might not write. With this commit, the `XException` of the write enable
is written as the value in the RAM, since it could have either been
written to or not been written to. On the next read of that address, it
will return the `XException`.

This issue did not propagate to `asyncRam` and `asyncRamPow2`, since
there, the same condition also causes the write address to be undefined,
and this is properly handled by the primitive.

3. The `asyncRAM#` primitive was also too strict in most of its inputs.
Combinatorially feeding the read output to the write-side inputs would
lock up the simulation, while it is a valid circuit. This problem did
not propagate to the `asyncRam` and `asyncRamPow2` functions, which are
lazy enough because the signals go through `Signal`'s `fmap` and `<*>`.

4. Data written to memory is `seqX`d for efficiency.

Additionally, documentation for memory components was harmonized and
corrected.
Copy link
Contributor

@alex-mckenna alex-mckenna left a comment

Choose a reason for hiding this comment

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

LGTM

@DigitalBrains1 DigitalBrains1 enabled auto-merge (squash) January 7, 2022 14:41
DigitalBrains1 added a commit that referenced this pull request Jan 7, 2022
PR #2006 was partially backported to 1.4 as #2031. For the changelog
shared with that backport, the changelog file is made identical to the
one from PR #2031.

The change in #2006 that was accidentally forgotten in the changelog is
added as a new file in this PR.
@DigitalBrains1 DigitalBrains1 mentioned this pull request Jan 7, 2022
2 tasks
DigitalBrains1 added a commit that referenced this pull request Jan 7, 2022
PR #2006 was partially backported to 1.4 as #2031. For the changelog
shared with that backport, the changelog file is made identical to the
one from PR #2031.

The change in #2006 that was accidentally forgotten in the changelog is
added as a new file in this PR.
@DigitalBrains1 DigitalBrains1 merged commit cf069ca into 1.4 Jan 7, 2022
@DigitalBrains1 DigitalBrains1 deleted the asyncram_multi_clk_fix_1.4 branch January 7, 2022 15:58
DigitalBrains1 added a commit that referenced this pull request Jan 26, 2022
The Haskell models of `blockRam#` and `blockRamFile#` treated an _undefined_
write enable as an asserted enable. But an _undefined_ value in Haskell can
correspond to any value whatsoever in HDL, so HDL simulation might or might not
write. With this commit, the `XException` of the write enable is written as the
value in the RAM, since it could have either been written to or not been
written to. On the next read of that address, it will return the `XException`.

This issue did not propagate to any other `blockRam` variants, the bug solely
manifested when using the `blockRam#` and `blockRamFile#` primitives directly.
All the other variants built upon those primitives always have their write
address undefined whenever the write enable is undefined, and that case was
properly handled by the primitive.

The issue is identical to one of the issues in PR #2006 and PR #2031,
for different memory primitives.
DigitalBrains1 added a commit that referenced this pull request Jan 27, 2022
The Haskell models of `blockRam#` and `blockRamFile#` treated an _undefined_
write enable as an asserted enable. But an _undefined_ value in Haskell can
correspond to any value whatsoever in HDL, so HDL simulation might or might not
write. With this commit, the `XException` of the write enable is written as the
value in the RAM, since it could have either been written to or not been
written to. On the next read of that address, it will return the `XException`.

This issue did not propagate to any other `blockRam` variants, the bug solely
manifested when using the `blockRam#` and `blockRamFile#` primitives directly.
All the other variants built upon those primitives always have their write
address undefined whenever the write enable is undefined, and that case was
properly handled by the primitive.

The issue is identical to one of the issues in PR #2006 and PR #2031,
for different memory primitives.
mergify bot pushed a commit that referenced this pull request Jan 27, 2022
The Haskell models of `blockRam#` and `blockRamFile#` treated an _undefined_
write enable as an asserted enable. But an _undefined_ value in Haskell can
correspond to any value whatsoever in HDL, so HDL simulation might or might not
write. With this commit, the `XException` of the write enable is written as the
value in the RAM, since it could have either been written to or not been
written to. On the next read of that address, it will return the `XException`.

This issue did not propagate to any other `blockRam` variants, the bug solely
manifested when using the `blockRam#` and `blockRamFile#` primitives directly.
All the other variants built upon those primitives always have their write
address undefined whenever the write enable is undefined, and that case was
properly handled by the primitive.

The issue is identical to one of the issues in PR #2006 and PR #2031,
for different memory primitives.

(cherry picked from commit ac97f0d)

# Conflicts:
#	clash-prelude/src/Clash/Explicit/BlockRam.hs
#	clash-prelude/src/Clash/Explicit/BlockRam/File.hs
#	clash-prelude/tests/Clash/Tests/BlockRam.hs
DigitalBrains1 added a commit that referenced this pull request Jan 30, 2022
The Haskell models of `blockRam#` and `blockRamFile#` treated an _undefined_
write enable as an asserted enable. But an _undefined_ value in Haskell can
correspond to any value whatsoever in HDL, so HDL simulation might or might not
write. With this commit, the `XException` of the write enable is written as the
value in the RAM, since it could have either been written to or not been
written to. On the next read of that address, it will return the `XException`.

This issue did not propagate to any other `blockRam` variants, the bug solely
manifested when using the `blockRam#` and `blockRamFile#` primitives directly.
All the other variants built upon those primitives always have their write
address undefined whenever the write enable is undefined, and that case was
properly handled by the primitive.

The issue is identical to one of the issues in PR #2006 and PR #2031,
for different memory primitives.

(cherry picked from commit ac97f0d)
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

2 participants