Generate correct SDC for Vivado

[NadiaYvette]

Xilinx' Vivado tcl tool rejects some identifiers generated by clash because they contain '$', so this avoids generating such identifiers.

Worked on at ZuriHac with @alex-mckenna
Fixes #2242

[martijnbastiaan]

This only adds a test case, perhaps you forgot a commit?

[alex-mckenna]

Actual fix is still WIP, I encouraged a push for visibility

[alex-mckenna]

Some initial thoughts, but overall this stayed a pretty small change like I hoped

[alex-mckenna]

The solution we picked didn't end up being vivado-specific, we can remove -fclash-hdlsyn vivado from the options

[alex-mckenna]

We can probably also test all hdlTargets, i.e. remove hdlTargets from here and just use the default. Since our fix isn't Verilog-specific either we would expect VHDL to also work this way. You would need to add mainVHDL and mainSystemVerilog to the test

[NadiaYvette]

A raft of cleanups of the issues you pointed out awaits further review, whenever you might have time. This one in particular

[alex-mckenna]

Another two comments. I think after this we can request a review from someone else to check that our idea / implementation is sensible.

[alex-mckenna]

@martijnbastiaan Since you already commented, you win the prize of being second reviewer

[NadiaYvette]

@alex-mckenna Let me know if there are other good issues to work on.

[alex-mckenna]

@NadiaYvette I think #1772 is also a pretty good first issue, this one will take you into clash-prelude (the standard library)

[martijnbastiaan]

Thanks @NadiaYvette for the effort, and thanks @alex-mckenna for the guidance. Super cool to see this coming out of ZuriHac. I've cut my review into two parts:

In general

I don't think this is the morally correct way of fixing this issue. What Vivado is complaining about is the name in bold (-name {c$arg}), not the part in italics (get_ports {c$arg}):

create_clock -name {c$arg} -period 5.000 -waveform {0.000 2.500} [get_ports {c$arg}]

This means that our HDL could remain as it is, we just need to think of sanitized names when rendering the TCL. E.g., we could emit:

create_clock -name {c_arg} -period 5.000 -waveform {0.000 2.500} [get_ports {c$arg}]

I think it'd therefore be neater if we'd go this way. The TCL would always work, flag or no flag.

This approach

Having said the previous - I still think it's worthwhile to have a flag like this. In the RealWorld(tm) we'll have to deal with broken tooling all the time, and Clash should be able to generate non-surprising, very simple identifier names. I do think we should change the flag name to -fclash-simple-basic-identifiers. "Sanitizing" doesn't really make sense to me, shouldn't Clash always generate "sane" identifier names? 😉

The complexity of this PR can be much reduced if you add the flag here:

clash-compiler/clash-lib/src/Clash/Netlist/Id.hs

Lines 98 to 108 in 925995c

	-- | Make a identifier set filled with given identifiers
	makeSet
	:: Bool
	-- ^ Allow escaped identifiers?
	-> PreserveCase
	-- ^ Should all basic identifiers be lower case?
	-> HDL
	-- ^ HDL to generate names for
	-> HashSet.HashSet Identifier
	-- ^ Identifiers to add to set
	-> IdentifierSet

The flag should then be used in make#:

clash-compiler/clash-lib/src/Clash/Netlist/Id/Internal.hs

Lines 118 to 131 in 925995c

	-- | Non-monadic, internal version of 'make'
	make# :: HasCallStack => IdentifierSet -> Text -> (IdentifierSet, Identifier)
	make# is0@(IdentifierSet esc lw hdl fresh0 ids0) (Common.prettyName -> id0) =
	if id1 `HashSet.member` ids0 then
	-- Ideally we'd like to continue with the id from the HashSet so all the old
	-- strings can be garbage collected, but I haven't found an efficient way of
	-- doing so. I also doubt that this case will get hit often..
	deepen# is0 id1
	else
	(is0{is_freshCache=fresh1, is_store=ids1}, id1)
	where
	ids1 = HashSet.insert id1 ids0
	fresh1 = updateFreshCache# fresh0 id1
	id1 = make## (is_hdl is0) (if esc then id0 else toBasicId# hdl lw id0)

There is precedence for a very similar flag, which enables/disables extended identifiers entirely, see -fclash-no-escaped-identifiers and the first argument to makeSet. Implementing it this way means users of Netlist.Id don't have to change.

As for the worries mentioned in the issue associated with this PR:

Changing the ID generation is indeed a little hairy here. The identifiers which cause problems ultimately come from unsafeFromCoreId, meaning we don't do the sanitising that we get from makeBasic etc.

This isn't true, all core ids are "sanitized" to valid HDL identifiers in mkUniqueNormalized (also see fromCoreId). Given that this isn't exactly type directed I'm sure there's bugs.. but that's not for this PR.

Again, thanks for the effort, let us know if you have any questions / need help!

[alex-mckenna]

I think it'd therefore be neater if we'd go this way. The TCL would always work, flag or no flag.

Right, that makes sense. I had read the issue as it not being fine with the identifier in any position, not just as the argument to -name.

I still think it's worthwhile to have a flag like this.

Yeah, perhaps a flag which limits basic identifiers to just [A-Za-z0-9_]+ (obviously while still following HDL-specific rules since you can make invalid identifiers already with just that)

all core ids are "sanitized" to valid HDL identifiers

I still don't particularly trust our name generation, but agreed that's not for this issue.

[martijnbastiaan]

I still don't particularly trust our name generation

I'll say this: if Clash.Netlist.Id ends up generating out-of-spec names I'll hand-deliver $favorite_drink to your doorstep^{terms and conditions apply}.