-fclash-aggressive-x-optimization-blackboxes does not work on deepErrorX of BitVector

[jonfowler]

The following code generates a reset on the data when compiled with clash -fclash-aggressive-x-optimization-blackboxes --vhdl:

import           Clash.Explicit.Prelude

topEntity
  :: Clock XilinxSystem
  -> Reset XilinxSystem
  -> Enable XilinxSystem
  -> Signal XilinxSystem (BitVector 8)
  -> Signal XilinxSystem (BitVector 8)
topEntity clk rst en = register clk rst en (deepErrorX "undefined reset")

vhdl:

  -- register begin
  result_1_register : process(clk)
  begin
    if rising_edge(clk) then
      if rst =  '1'  then
        result_1 <= std_logic_vector'("--------"); -- OFFENDING LINE
      elsif en then
        result_1 <= i;
      end if;
    end if;
  end process;
  -- register end

Replacing BitVector for Unsigned does not have this problem.

Presumably this is because of a BitVector creates a mask not a direct errorX and presumably similar problems will exist with records where every field is errorX but the structure is defined. The intuitive fix would be to pack the data and check whether it is all undefined as opposed to relying on the Haskell representation.

[martijnbastiaan]

This is checked in the blackbox templates like so:

clash-compiler/clash-lib/prims/vhdl/Clash_Signal_Internal.primitives.yaml

Line 92 in 0b4a9ae

~IF ~ISUNDEFINED[6] ~THEN ~ELSEif ~ARG[3] = ~IF ~ISACTIVEHIGH[0] ~THEN '1' ~ELSE '0' ~FI then

We should special case the implementation of ~ISUNDEFINED for BitVector, over here:

clash-compiler/clash-lib/src/Clash/Netlist/BlackBox/Util.hs

Lines 476 to 480 in 0b4a9ae

	(IsUndefined n) ->
	let (e, _, _) = bbInputs b !! n in
	case (xOpt, e) of
	(True, BlackBoxE _ _ _ _ (N.BBTemplate [Err _]) _ _) -> 1
	_ -> 0

[jonfowler]

@martijnbastiaan it would be nice to have something which also worked for something like this:

import           Clash.Explicit.Prelude

topEntity
  :: Clock XilinxSystem
  -> Reset XilinxSystem
  -> Enable XilinxSystem
  -> Signal XilinxSystem (Bit, Bit)
  -> Signal XilinxSystem (Bit, Bit)
topEntity clk rst en = register clk rst en (deepErrorX "undefined reset")

But I see that is potentially difficult as this unpacks to:

-- Automatically generated VHDL-93
library IEEE;
use IEEE.STD_LOGIC_1164.ALL;
use IEEE.NUMERIC_STD.ALL;
use IEEE.MATH_REAL.ALL;
use std.textio.all;
use work.all;
use work.Main_topEntity_types.all;

entity topEntity is
  port(-- clock
       clk      : in Main_topEntity_types.clk_XilinxSystem;
       -- reset
       rst      : in Main_topEntity_types.rst_XilinxSystem;
       -- enable
       en       : in Main_topEntity_types.en_XilinxSystem;
       i_0      : in std_logic;
       i_1      : in std_logic;
       result_0 : out std_logic;
       result_1 : out std_logic);
end;

architecture structural of topEntity is
  signal result_4 : Main_topEntity_types.Tup2 := (Tup2_sel0_std_logic_0 => '-', Tup2_sel1_std_logic_1 => '-');
  signal i        : Main_topEntity_types.Tup2;
  signal result   : Main_topEntity_types.Tup2;

begin
  i <= ( Tup2_sel0_std_logic_0 => i_0
       , Tup2_sel1_std_logic_1 => i_1 );

  -- register begin
  result_4_register : process(clk)
  begin
    if rising_edge(clk) then
      if rst =  '1'  then
        result_4 <= (Tup2_sel0_std_logic_0 => '-', Tup2_sel1_std_logic_1 => '-');
      elsif en then
        result_4 <= i;
      end if;
    end if;
  end process;
  -- register end

  result <= result_4;

  result_0 <= result.Tup2_sel0_std_logic_0;

  result_1 <= result.Tup2_sel1_std_logic_1;


end;

[martijnbastiaan]

I think this would be a matter of recursing on

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

Lines 440 to 442 in 0b4a9ae

	| Product !Text (Maybe [Text]) [HWType]
	-- ^ Product type: Name, field names, and field types. Field names will be
	-- populated when using records.

in the case I linked earlier, would you agree?

[alex-mckenna]

I wouldn't do that for aggressive blackbox X optimization though. Digging into product types seems like something the normal aggressive X optimization flag should do since it's just data

[martijnbastiaan]

Are you proposing that ~ISUNDEFINED on (_|_, _|_) would only return True if -fclash-aggressive-x-optimization and -fclash-aggressive-x-optimization-blackboxes? That seems counter-intuitive to me. The flag's documentation heavily implies that deepErrorX should just work:

Allow blackboxes to detect undefined values and change their behavior accordingly. For example, if register is used in combination with an undefined reset value, it will leave out the reset logic entirely.

[alex-mckenna]

No, I'm saying it should be -fclash-aggressive-x-optimization instead of -fclash-aggressive-x-optimization-blackboxes, since it has nothing to do with blackboxes. i.e. a PR with

• a commit to add the special case for BitVector, ideally with a comment saying that it is needed since BitVector pretends to have a spine for deepErrorX (since it's counter-intuitive to an extent)
• a commit to allow -fclash-aggressive-x-optimization to also look through product types and see if all sub-data is undefined when doing X optimization

EDIT: No, this is wrong. The docs for -fclash-aggressive-x-optimization-blackboxes say that it's all about ~ISUNDEFINED, so you're right @martijnbastiaan 🙃

[martijnbastiaan]

While that's certainly nice to have, I fail to see how that would help in this specific instance. The reset value of register would still end up as (_|_, _|_) in the netlist.

[christiaanb]

Or the users should use autoReg which splits tuples automatically. Then ~ISUNDEFINED doesn't have to recurse through product types.

[martijnbastiaan]

I think both should work. I wouldn't say it's unreasonably to treat errorX and deepErrorX as isomorphic in the netlist.

@jonfowler We quickly discussed this in the office just now; if you supply a patch we'll accept it :)

[jonfowler]

Thanks, but I'm not going to have time to do this at the moment (it should be a minor change but being unfamiliar with the codebase means it's going to take me quite a while to investigate / do this properly).

[DigitalBrains1]

I'll give it a go. Reviewer beware: also not really my area of expertise ;-)

[martijnbastiaan]

We've released v1.6.3, which includes a fix for this issue.