Refactor DEC transformation

[christiaanb]

Create multiple selectors, one for each non-shared argument.
We still use one selector in order to preserve sharing.

The previous code was a big mess where we partioned arguments into shared and non-shared and then filtered the case-tree depending on whether they were part of the shared arguments or not. But then with the normalisation of type arguments, the second filter did not work properly. This then resulted in shared arguments becoming part of the tuple in the alternatives of the case-expression for the non-shared arguments.

The new code is also more robust in the sense that shared and non-shared arguments no longer need to be partioned (shared occur left-most, non-shared occur right-most). They can now be interleaved. The old code would also generate bad Core if ever type and term arguments occured interleaved, this is no longer the case for the new code.

Fixes #2628

Still TODO:

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

[martijnbastiaan]

Not a useful review at all, just a style suggestion..

[leonschoorl]

That is much cleaner.
And now it actually does what is written in the comment above it.

Given:

topEntity x y z = case g z of
  A -> f 3 y
  B -> f x x
  C -> h x

This new version now produces:

let
  f_out = let djArg0 = case g z of { A -> 3; B -> x }
              djArg1 = case g z of { A -> y; B -> x }
          in f djArg0 djArg1
in case g z of
     A  -> f_out
     B  -> f_out
     C  -> h x

Where the old version produced:

let
  f_out = let tupIn = case g z of
                        A  -> (3, y)
                        B  -> (x, x)
          in f (case tupIn of (x, _) -> x)
               (case tupIn of (_, x) -> x)
in case g z of
     A  -> f_out
     B  -> f_out
     C  -> h x

The old version worked hard to keep the number of case statements low in HDL (projections don't count).
It would generate a single case statement, whose result was a tuple with one element for each (non-shared) argument.
It seems that despite what the comment says the DEC transformation has done this since its beginning.

I'm unsure whether that was an important property or not.
I can imagine that less case statements might produce better hardware, even if they're wider.
But the opposite might also be true, have fewer very wide case statements could pull things together and increase routing contention.
But splitting a case statement seems like an easier operation than combining.
Maybe I'm just way over thinking things.

The old version would always duplicate the case subject once.
But this new version duplicates the case subject once for every (non-shared) argument.
In this example CSE will later deduplicate the case subjects.
But maybe it makes sense to do that directly in the DEC here?
If DEC does anything we know that we're duplicating the case subject at least once, so we could easily just create a let binding for it.
(Unless it's already a Var, or is static or possibly workfree)

[christiaanb]

Two multiplexers of width X results in just as much logic as one multiplexer of width X + X. So whether we have one case-statement over a product type, or multiple case-statements over the individual types, results in the same size circuit.

You are correct that the subject of the case-expressions is duplicated by the new code. Deduplication does lead to a higher fan-out, and is something that can be controled by deDup and noDeDup. I guess it doesn't hurt the complexity of this transformation too much to already do the deduplication of subject here instead of waiting for the CSE transformation to do it.

[leonschoorl]

simpleMux :: Unsigned 2 -> (t,t,t,t) -> t
simpleMux sel (a,b,c,d) = case sel of { 0 -> a; 1 -> b; 2 -> c; 3 -> d }

wideSubject :: Unsigned 256 -> (t,t,t,t) -> t
wideSubject sel (a,b,c,d) = case sel of { 0 -> a; 1 -> b; 0xaabbaabbaabbaabbaabbaabbaabbaabbaabbaabbaabbaabbaabbaabbaabbaabb -> c; _ -> d }

I agree that for something like simpleMux having many narrow muxes or one wide one is basically equivalent.

But I thought the story might be different for something like wideSubject.

[leonschoorl]

I did an experiment.

-- make one big wide mux
f :: forall t sel. (sel -> (t,t,t,t) -> t) -> sel -> (t,t,t,t) -> t
f = id

-- make many 1 bit wide muxes
g :: forall ts sel. BitPack ts => (forall t. sel -> (t,t,t,t) -> t) -> sel -> (ts,ts,ts,ts) -> ts
g mux sel (a,b,c,d) = bitCoerce @(Vec (BitSize ts) Bit) @ts $ fmap (mux sel) $ zip4 as bs cs ds
 where
  as = bitCoerce a :: Vec (BitSize ts) Bit
  bs = bitCoerce b :: Vec (BitSize ts) Bit
  cs = bitCoerce c :: Vec (BitSize ts) Bit
  ds = bitCoerce d :: Vec (BitSize ts) Bit


fWide,gWide :: Unsigned 256 -> (Unsigned 40, Unsigned 40, Unsigned 40, Unsigned 40) -> Unsigned 40
fWide =  f wideSubject
gWide =  g wideSubject

Vivado does synthesize to them to the different muxes:

fWide:
+---Muxes : 
	   4 Input   40 Bit        Muxes := 1     
	   4 Input    2 Bit        Muxes := 1     

gWide:
+---Muxes : 
	   4 Input    2 Bit        Muxes := 40    
	   4 Input    1 Bit        Muxes := 40   

But in the end both had the exact same cell usage:

+------+-----+------+
|      |Cell |Count |
+------+-----+------+
|1     |LUT4 |    68|
|2     |LUT5 |    36|
|3     |LUT6 |    88|
|4     |IBUF |   416|
|5     |OBUF |    40|
+------+-----+------+

[christiaanb]

I've changed the code so that it generates a single tuple again, in order to preserve sharing of the subject of the case-statement

[leonschoorl]

+FIXED: Clash errors out in the netlist-generation stage when ...

?

[martijnbastiaan]

Also, "netlist" and "generation" should probably not be hyphenated.

[DigitalBrains1]

Neither should case-statement or new-type

Suggested change

	FIXED: Clash errors our in netlist-generation stage out when a polymorphic function is applied to type X in one alternative of a case-statement and applied to a new-type wrapper of type X in a different alternative. See [#2828](https://github.com/clash-lang/clash-compiler/issues/2628)
	FIXED: Clash no longer errors out in the netlist generation stage when a polymorphic function is applied to type X in one alternative of a case statement and applied to a newtype wrapper of type X in a different alternative. See [#2828](https://github.com/clash-lang/clash-compiler/issues/2628)

Also, I am to understand that Clash now no longer errors out? That's not what it said: Fixed: Clash errors out. Without emphasis, I would interpret that as "Clash didn't use to error out, this is now fixed so it properly errors out." However, I believe that is the wrong interpretation. So I changed it to "no longer errors out" to emphasise this. If instead it's meant that it now errors out, it should say "Clash now errors out". The old phrasing was just too ambiguous.

[edit] Added an article in front of netlist. The rest of the sentence doesn't omit articles either. [/edit]

[martijnbastiaan]

We currently mix: FIXED: {issue subject} and FIXED: {changelog styled entry}, which is confusing. We should pick one, yeah.

[DigitalBrains1]

If we make sure it can only be interpreted one way (now, no longer), people won't have to analyse the other entries in the changelog to see which style we use. While it may not be pretty if we mix the two styles you mention, we can at least prevent ambiguity by taking an adversarial approach to reading our own changelog entries: can I actually read it in a different way? If so, rephrase.

[christiaanb]

But this changelog entry will become part of a list:

FIXED:
* defect description 1
* defect description 2
* defect description 3
* etc.

Perhaps instead of FIXED, it should say BUGS FIXED.

I can chance the description so that it says:

Clash falsely errors out during netlist generation when a polymorphic function is applied to type X in one alternative of a case-statement and applied to a newtype wrapper of type X in a different alternative. See [#2828](https://github.com/clash-lang/clash-compiler/issues/2628

[DigitalBrains1]

But why do you keep insisting on the present tense? By the time people read the changelog, the behaviour will be in the past. Can you give me a proper sentence where the problem is described in the present tense? This present tense only serves to confuse. Not only is it wrong, it is confusing, because it is wrong.

[edit]

But this changelog entry will become part of a list

You're arguing something I conceded to you several messages ago already. I don't care! I just want it to be clear.
[/edit]

[christiaanb]

Because people most interested in the "bugs fixed" list are the ones that have a bug they are experiencing presently, and want to know whether it is fixed by the new version; and thus whether they should upgrade from e.g. version 1.6.6 to version 1.8.4

[DigitalBrains1]

It is confusing, and therefore it gets a NAK from me. I don't care how you phrase it as long as it is not confusing. This is confusing. The intent is to communicate clearly.

[edit]
Additionally, you should rename it FIXES, because FIXED is very clearly past tense.
[/edit]

[DigitalBrains1]

If you rewrite our complete current changelog to no longer be:

Added:

• ...

Removed:

• ...

Deprecated:

• ...

Changed:

• ...

Fixed:

• ...

etcetera but:

Adds:

• ...

Removes:

• ...

Deprecates:

• ...

Changes:

• ...

Fixes:

• ...

etcetera, then the present tense is no longer confusing and I'll happily accept it. As it is now, it is different than most entries, different even than its own heading, and that is confusing.

[leonschoorl]

Confusingly the argument order is reversed:

Suggested change

	-- let tupIn = case x of {A -> (3,y); B -> (x,x)}
	-- f_arg0 = case tupIn of (l,_) -> l
	-- f_arg1 = case tupIn of (_,r) -> r
	-- f_out = f f_arg0 f_arg1
	-- let tupIn = case x of {A -> (y,3); B -> (x,x)}
	-- f_arg1 = case tupIn of (r,_) -> r
	-- f_arg0 = case tupIn of (_,l) -> l
	-- f_out = f f_arg0 f_arg1

[leonschoorl]

To avoid confusing when reading the resulting core, it would probably be best to change the code so does what you wrote here.

[martijnbastiaan]

I don't really have the bandwidth to properly review this. Given that you and Leon went over it in serious detail, I'd feel comfortable merging this.

[martijnbastiaan]

Also, "netlist" and "generation" should probably not be hyphenated.

[leonschoorl]

Two issues are still open: the whole changelog entry discussion
and the example not matching what is actually happening

Otherwise LGTM

[christiaanb]

@leonschoorl @DigitalBrains1 Could you have a look at the new version? I believe it addresses your concerns.

[DigitalBrains1]

Changelog LGTM apart from this little detail:

Suggested change

	FIXED: Clash no longer errors out in the netlist generation stage out when a polymorphic function is applied to type X in one alternative of a case-statement and applied to a newtype wrapper of type X in a different alternative. See [#2828](https://github.com/clash-lang/clash-compiler/issues/2628)
	FIXED: Clash no longer errors out in the netlist generation stage when a polymorphic function is applied to type X in one alternative of a case-statement and applied to a newtype wrapper of type X in a different alternative. See [#2828](https://github.com/clash-lang/clash-compiler/issues/2628)

[christiaanb]

Thanks, I'll fix that :)