Fallible unpacking by rslawson · Pull Request #3177 · clash-lang/clash-compiler

rslawson · 2026-04-05T14:08:19Z

As per #3150, this is an implementation of fallible unpacking (BitVector (BitSize a) -> Maybe a). This is largely separate from the existing unpack implementations, except where those can be relied upon for easy implementations (maybeUnpack = Just . unpack).

Still TODO:

Write a changelog entry (see changelog/README.md)
Check copyright notices are up to date in edited files
Ensure that the new feature is tested properly

martijnbastiaan

@rslawson Not a real review yet, just wanted to drop by to say that it's awesome that you're working on this :). It seems very sensible to me to add this as a separate maybeUnpack function that's a member of BitPack.

martijnbastiaan · 2026-04-05T18:33:35Z

+
+{-# INLINE maybeFromInteger_INLINE #-}
+maybeFromInteger_INLINE :: forall n . (HasCallStack, KnownNat n) => Integer -> Maybe (Index n)
+maybeFromInteger_INLINE i = bound `seq` if i > (-1) && i < bound then Just (I i) else Nothing


Huh, why the seq?

Entirely because I copied fromInteger_INLINE and just changed a couple things. @rowanG077 suggested when I talked with him in the office that I should probably rewrite that function in terms of this one, but I forgot to do that before committing this.

The seq is there in fromInteger_INLINE and was copied here I guess. I'm not sure why it's there either. It was introduced in 8ca93af but the only thing the commit says is A more readable implementation of Index.fromInteger

martijnbastiaan · 2026-04-05T18:38:02Z

+      otherwiseNothing = [(NormalG (ConE 'True), ConE 'Nothing)]
+      unpackBody       = MultiIfE (justMatches ++ otherwiseNothing)
+      unpackLambda     = LamE [VarP argName] unpackBody
+      unpackApplied    = (VarE 'dontApplyInHDL) `AppE` unpackLambda `AppE` (VarE (argNameIn))


Any reason you're using magic like dontApplyInHDL? Just naively I would expect the functionality of this to be entirely implementable without it.

It's there from copying over parts of the code in buildUnpack. I wasn't sure which parts I should or shouldn't keep, so I just went with the strategy of take what I can if it doesn't break things.

The source of that dontApplyInHdl is this commit (part of this PR, the introduction of custom bit-representations).

While it does clean up the generated HDL, it is also the only occurrence I know of so far that requires BitPack to have the same bit representation as the type itself, causing errors if it is not.

rowanG077 · 2026-04-16T06:41:34Z

To give an update on this:

I want to do some tests with yosys locally to see what kind of resource usage we can expect from maybeUnpack. I'm also curious if now fromJustX . maybeUnpack will actually make it free.

rowanG077 · 2026-05-26T11:36:26Z

I tested bittide by replacing unpack with fromJustX . maybeUnpack and these are the results when synthesizing it for an ECP5 target. The proprietary blackboxes are stubbed out.

  ┌─────────────────┬──────────┬─────────┬───────┐
  │ Resource        │ Baseline │ Patched │ Delta │
  ├─────────────────┼──────────┼─────────┼───────┤
  │ LUT4            │    64801 │   65014 │  +213 │
  │ TRELLIS_FF      │    45452 │   45452 │     0 │
  │ CCU2C           │     5939 │    5926 │   -13 │
  │ L6MUX21         │      305 │     372 │   +67 │
  │ PFUMX           │     5869 │    5857 │   -12 │
  │ DP16KD          │      826 │     826 │     0 │
  │ TRELLIS_DPR16X4 │      132 │     132 │     0 │
  │ MULT18X18D      │       12 │      12 │     0 │
  └─────────────────┴──────────┴─────────┴───────┘

So it is a regression. Testing separately does show it does optimize things out correctly but it's probably not always possible in larger structures. For me this PR can now be merged. I will retag the reviewers.

martijnbastiaan · 2026-05-26T11:43:32Z

@rowanG077 Do you have a top entity you can share? I'll push it through Vivado.

DigitalBrains1 · 2026-05-26T16:58:41Z

-  unpack = checkUnpackUndef unpackChar#
+  pack        = packXWith packChar#
+  unpack      = checkUnpackUndef unpackChar#
+  maybeUnpack = Just . unpack


>>> unpack 0x110000 :: Char *** Exception: Prelude.chr: bad argument: 1114112

Unicode code points lie in the range U+0000 to U+D7FF and U+E000 to U+10FFFF. It seems Haskell isn't too concerned with the gap, it doesn't error for values in that range that I tried. But it does bottom with an ErrorCall for numbers larger than 0x10ffff. Not an XException, mind you.

I think it'd be better if maybeUnpack returned Nothing for out-of-range numbers.

Values in the gap round-trip neatly in GHC 9.10.3:

>>> all (\x -> x == (ord $ chr x)) [0xd800 .. 0xdfff] True

Agree Peter! This is also one of the rules of NumConvert: succes -> not bottom. It makes a lot of sense to make conversion functions do this across the prelude.

DigitalBrains1 · 2026-05-26T17:05:06Z

Ah, this is a very welcome addition, thanks! I've only quickly scrolled through so far, I'll give it a bit more time later.

rowanG077 · 2026-05-28T13:53:50Z

@martijnbastiaan Yes

The maybeUnpack.zip contains the clash generated files for unpack = fromJustX . maybeUnpack. unpack.zip is the baseline.

maybeUnpack.zip
unpack.zip

DigitalBrains1

Looking good! Aside from the true issue with maybeUnpack for Char I already commented on, I just wonder why a test is what it is, have a small suggestion regarding an error message and I wanted to show the reader what a piece of TH does, just as information.

DigitalBrains1 · 2026-05-28T14:52:37Z

            ]

-    in InstanceD Nothing context instTy [bitSizeType, pack, unpack]
+        maybeUnpack =


Hah, you go in thinking it'll just be very similar to unpack but with a Functor (Maybe _) like so many other instances, and it turns out to be a lot more complicated! Well, in TH. In Haskell it's still very basic, but still.

This definition looks fine, I was just wondering why it was more complicated than others and understand it now. The TH is still somewhat readable, but I asked GHC to read it for me so it becomes more readable. Just for reference, -ddump-splices tells us the three-tuple is:

instance (BitPack a, KnownNat (BitSize a), BitPack (a0, a1), KnownNat (BitSize (a0, a1))) => BitPack (a, a0, a1) where type BitSize (a, a0, a1) = (+) (BitSize a) (BitSize (a0, a1)) pack tup = pack (retup tup) where retup (a, a0, a1) = (a, (a0, a1)) unpack x = let (a, y) = unpack x (a0, a1) = unpack y in (a, a0, a1) maybeUnpack x = let bvL :: BitVector (BitSize a) bvR :: BitVector (BitSize (a0, a1)) (bvL, bvR) = split# x in case (maybeUnpack bvL, maybeUnpack bvR) of (Just a, Just (a0, a1)) -> Just (a, a0, a1) _ -> Nothing

I've cleaned up the names for readability.

I just started wondering why you give bvL and bvR type signatures. Surely from the fact that they unpack to a and (a0, a1), GHC should be able to imply where to split# the bit vector? I copied the code in the previous message in the place of deriveBitPackTuples, removed the type signatures of bvL and bvR and sure enough it compiles and works:

maybeUnpack x = let (bvL, bvR) = split# x in case (maybeUnpack bvL, maybeUnpack bvR) of (Just a, Just (a0, a1)) -> Just (a, a0, a1) _ -> Nothing

so it's possible to simplify this TH AST a bit.

DigitalBrains1 · 2026-05-28T15:07:17Z

+maybeUnpack# :: forall n. KnownNat n => BitVector (CLogWZ 2 n 0) -> Maybe (Index n)
+maybeUnpack# bv
+  | bound == 0 = Nothing
+  | bv <= maxBoundBV = Just (unpack# bv)


I think this is good enough, but it is a bit of a pity that the error message isn't as straight-forward as the unpack case:

>>> pack (maxBound :: Index 3) 0b10 >>> let bv = $(bLit "0.") >>> unpack bv :: Index 3 *** Exception: X: Index.unpack called with (partially) undefined arguments: 0b0. >>> maybeUnpack bv :: Maybe (Index 3) *** Exception: X: Clash.Sized.BitVector.<= called with (partially) undefined arguments: 0b0. <= 0b10

Do we want to improve the error message with something funky like

maybeUnpack# bv | bound == 0 = Nothing | !i <- unpack# bv , bv <= maxBoundBV = Just i | otherwise = Nothing where bound = natToInteger @n maxBoundBV = pack# (maxBound# @n)

?

That way maybeUnpack# still doesn't require a black box. The error message is still referencing Index.unpack though. We could also use clashSimulation to only dive into the BV constructor in simulation and use the code as it is now for HDL generation! That might be even better.

Wait, I'm being silly, by forcing unpack# to WHNF before checking for out-of-bounds, it will bottom for out-of-bounds values through the check in fromInteger_INLINE.

But the following works in simulation and produces nice HDL (tested in GHC 9.12.4):

maybeUnpack# bv | bound == 0 = Nothing | clashSimulation , BV mask _ <- bv , mask > 0 = undefError "Index.maybeUnpack" [bv] | bv <= maxBoundBV = Just (unpack# bv) | otherwise = Nothing where bound = natToInteger @n maxBoundBV = pack# (maxBound# @n)

DigitalBrains1 · 2026-05-28T15:22:42Z

+assertNothing label (Just _) =
+  P.error (label P.<> ": expected Nothing, but got Just _")
+
+mainCommon :: IO ()


Doesn't this outputTest just do some stuff that can be done in clash-prelude/tests/Clash/Tests/BitPack.hs, and in fact it already overlaps with that? I think any non-overlap is better put in the Tasty suite over in clash-prelude.

Made in response to the discussion post at #3150

martijnbastiaan reviewed Apr 5, 2026

View reviewed changes

rowanG077 force-pushed the rs/maybeUnpack branch 2 times, most recently from a5598eb to 203dda6 Compare April 5, 2026 22:35

rowanG077 force-pushed the rs/maybeUnpack branch from 203dda6 to f9fb2e7 Compare May 26, 2026 08:42

rowanG077 requested review from DigitalBrains1 and martijnbastiaan May 26, 2026 11:36

DigitalBrains1 reviewed May 26, 2026

View reviewed changes

rowanG077 force-pushed the rs/maybeUnpack branch 2 times, most recently from af4aad7 to 797dfe6 Compare May 28, 2026 13:50

DigitalBrains1 reviewed May 28, 2026

View reviewed changes

rslawson and others added 6 commits June 4, 2026 13:51

Implement fallible unpacking (maybeUnpack, gMaybeUnpack)

36a149b

Made in response to the discussion post at #3150

Add test for undefined bits mirroring UnpackUndefined.hs

8e36a48

Add some simulation tests

7aac60d

Fix maybeUnpack for Index

d03892a

Add some more test

4923a06

Fix BitPack deriving TH for maybeUnpack

6e7a931

martijnbastiaan force-pushed the rs/maybeUnpack branch from 797dfe6 to 6e7a931 Compare June 4, 2026 11:54

Conversation

rslawson commented Apr 5, 2026

Still TODO:

Uh oh!

martijnbastiaan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rowanG077 commented Apr 16, 2026

Uh oh!

rowanG077 commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martijnbastiaan commented May 26, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DigitalBrains1 commented May 26, 2026

Uh oh!

rowanG077 commented May 28, 2026

Uh oh!

DigitalBrains1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DigitalBrains1 May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DigitalBrains1 May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DigitalBrains1 May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rowanG077 commented May 26, 2026 •

edited

Loading

DigitalBrains1 left a comment •

edited

Loading

DigitalBrains1 May 28, 2026 •

edited

Loading

DigitalBrains1 May 28, 2026 •

edited

Loading

DigitalBrains1 May 28, 2026 •

edited

Loading