Mark primitives OPAQUE and update to clash-prelude-1.10 - #14
Open
felipebalbi wants to merge 2 commits into
Open
Conversation
Clash 1.10 warns on every primitive it loads from this package:
Warning: primitive Ice40.Rgb.rgbPrim isn't marked OPAQUE.
This might make Clash ignore this primitive.
GHC 9.4 introduced OPAQUE precisely for this case: NOINLINE stops inlining
but still permits the simplifier to rewrite a binding in ways that can break
blackbox matching, whereas OPAQUE also disables those transformations.
clash-lib's isOpaque accepts only Opaque, so NOINLINE trips the warning for
all twelve primitives here, not just the RGB one.
The warning cannot be silenced from the user's side. -fclash-no-prim-warn
does not cover it: clash-ghc emits it via an unguarded warnIf in
GenerateBindings.hs, and opt_primWarn only guards "Dubious primitive
instantiation" in clash-lib's Netlist/BlackBox.hs.
CI builds GHC 9.0, which has no OPAQUE, so each pragma is CPP-guarded and
9.0 keeps NOINLINE. That is not a downgrade: on a GHC without OPAQUE there
is nothing stricter to ask for.
Verified on GHC 9.10.3 / Clash 1.10.0 against a design that instantiates
SB_RGBA_DRV: the warning is gone and the generated Verilog is byte-identical
before and after (md5 dc4bb4b391125dda03c4f6097b670c85), so this changes
what GHC is permitted to do, not what Clash emits.
The upper bounds stopped at clash-prelude < 1.9 and base < 4.20, so the package would not resolve against Clash 1.10 on GHC 9.10. Nothing in it actually breaks: with the bounds relaxed the library builds clean under -Wall -Wcompat with no warnings, and the emitted Verilog is unchanged. Bounds follow the granularity this package already uses -- 1.6.1 was admitted with < 1.7 and 1.8 with < 1.9, so 1.10 gets < 1.11. base < 4.21 covers GHC 9.10. CI gains 9.8 and 9.10 so these bounds are tested rather than asserted; claiming an untested upper bound is how the previous one went stale. 9.0 and 9.4 still resolve older clash-prelude, which is why the lower bounds are untouched. Verified on GHC 9.10.3 / clash-prelude 1.10.0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mark primitives
OPAQUEon GHC ≥ 9.4, and support clash-prelude 1.10 / GHC 9.10Two independent changes, one commit each, so you can take either on its own.
1.
NOINLINE→OPAQUE(2d85eac)Clash 1.10 warns on every primitive this package exports:
Warning: primitive Ice40.Rgb.rgbPrim isn't marked OPAQUE.
This might make Clash ignore this primitive.
clash-lib'sisOpaqueaccepts onlyOpaque, so all twelve primitives trip it —Spram,Spi,Rgb,Pll.Pad,Pll.Core,Osc(×2),Mac.Prim,Led,IO,I2c,GB— not just the one a given design happens to use.GHC 9.4 added
OPAQUEfor exactly this case.NOINLINEprevents inlining but stilllets the simplifier rewrite a binding in ways that can break blackbox matching;
OPAQUEalso disables those transformations. For a primitive whose whole contract is"match this name and emit the template", that is the stronger and more accurate
statement of intent.
It can't be silenced downstream.
-fclash-no-prim-warndoesn't cover it —clash-ghcemits this one through an unguardedwarnIfinGenerateBindings.hs,while
opt_primWarnonly guards "Dubious primitive instantiation" inclash-lib'sNetlist/BlackBox.hs. Confirmed by reading both sources and by running with the flag.Why CPP. CI builds GHC 9.0, which has no
OPAQUE, so each pragma is guarded and9.0 keeps
NOINLINE. That isn't a downgrade — on a GHC withoutOPAQUEthere'snothing stricter available.
2. Dependency bounds (
6cdec17)base < 4.20→< 4.21,clash-prelude < 1.9→< 1.11. Nothing in the packageactually breaks on the newer versions; the bounds were simply the binding constraint.
Granularity follows this package's own precedent: 1.6.1 was admitted with
< 1.7and1.8 with
< 1.9, so 1.10 gets< 1.11.CI gains GHC 9.8 and 9.10 so the new bounds are tested rather than asserted — an
untested upper bound is how the previous one went stale, and I'd rather not repeat
that. 9.0 and 9.4 still resolve older
clash-prelude, so the lower bounds areuntouched.
Verification
On GHC 9.10.3 / clash-prelude 1.10.0:
-Wall -Wcompat, all 13 modulesallow-newerSB_RGBA_DRVdc4bb4b391125dda03c4f6097b670c85That last row is the important one: this changes what GHC is permitted to do, not
what Clash emits.
Notes
## Unreleased— releasetiming is yours to decide. Happy to fold it into a
0.3.1.5heading instead.ubuntu-20.04, which GitHub has retired, so the workflow may not bestarting at all today regardless of this PR. I left it alone as out of scope, but say
the word and I'll bump it to
ubuntu-latesthere.cabal checkreports missing upper bounds onghc-typelits-{natnormalise,extra,knownnat}.Pre-existing, untouched.