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.

Sign up for GitHub

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

Jump to bottom

__VOID_TDECL_NOOP__ is not correctly matched #2654

Open
kleinreact opened this issue Feb 2, 2024 · 2 comments · May be fixed by #2662
Open

__VOID_TDECL_NOOP__ is not correctly matched #2654

kleinreact opened this issue Feb 2, 2024 · 2 comments · May be fixed by #2662

Comments

@kleinreact
Copy link
Member

As discovered during the discussion of #2649 by @christiaanb, the following pattern match does not seem to work correctly

clash-compiler/clash-cores/src/Clash/Cores/Xilinx/Ila/Internal.hs

Line 278 in 5e01ae2

getIlaName (Just "__VOID_TDECL_NOOP__") = getIlaName Nothing
as the following example

module Test where

import Clash.Explicit.Prelude
import Clash.Cores.Xilinx.Ila

topEntity :: Clock System -> Signal System ()
topEntity clk =
    hwSeqX (ilaInst (ilaConfig ("a" :> Nil)))
  $ hwSeqX (ilaInst (ilaConfig ("b" :> Nil)))
    pure ()
 where
  ilaInst :: IlaConfig 1 -> Signal System ()
  ilaInst cfg = ila cfg clk (pure 0 :: Signal System (Unsigned 1))

still produces

...
topEntity_ila_E981CE816A1232C3 topEntity_probe___VOID_TDECL_NOOP___0
  (.clk (c$pTS0), .probe0 (a));
...
topEntity_ila_E981CE816A1232C3 topEntity_probe___VOID_TDECL_NOOP__
  (.clk (c$pTS0), .probe0 (b));
...
@DigitalBrains1
Copy link
Member

That's because the name is Just "topEntity_probe___VOID_TDECL_NOOP__", not just the suffix.

@DigitalBrains1
Copy link
Member

DigitalBrains1 commented Feb 4, 2024 •
edited
Loading

This is related to my comment in #2655: as soon as Clash needs to deduplicate, as here, it will prefix the name with a prefix based on name of the entity containing the ILA. At that point the pattern no longer matches. The pattern was probably only tested with a single ILA.

If I only leave one ILA in your code above, I get this bbCtxName:

Just "__VOID_TDECL_NOOP__"

but with the original code with two ILAs they are:

Just "topEntity_ilaInst___VOID_TDECL_NOOP__"
Just "topEntity_ilaInst___VOID_TDECL_NOOP__"

Note they haven't been deduplicated yet.

[edit]
Huh... the prefix isn't the same as yours. I'm going to guess we are using different versions of GHC and the Core contains subtly different names. I used GHC 9.6.3 on latest Clash master.
[/edit]

DigitalBrains1 added a commit that referenced this issue Feb 8, 2024
@DigitalBrains1
VIO and ILA: explicitly check setName
87f39a7 
Use the specified name if there is a `setName` tick, or the default name
otherwise.

Fixes #2654
DigitalBrains1 added a commit that referenced this issue Feb 8, 2024
@DigitalBrains1
VIO and ILA: explicitly check setName
d5a8864 
If `Clash.Magic.setName` was used, use that name for the instance.
Otherwise, use a fixed default name.

Fixes #2654
@DigitalBrains1 DigitalBrains1 linked a pull request Feb 8, 2024 that will close this issue
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

VIO and ILA: explicitly check setName clash-lang/clash-compiler
2 participants
@DigitalBrains1 @kleinreact