vhdl shift_left bounds problem with 1.6.3

[pbreuer]

Another vhdl problem, but is it ghdl or is it clash? The runtime error reported from ghdl is:

./kpu_test:error: bound check failure at Test_Trace_kpu_test32_execA.vhdl:9809

The lines are:

cds_app_arg_2_shiftL : block
  signal sh_7 : natural;
begin
  sh_7 <=
      -- pragma translate_off
      natural'high when (\c$shI_4\(64-1 downto 31) /= 0) else
      -- pragma translate_on
      to_integer(\c$shI_4\               <------ line 9809 HERE!
      -- pragma translate_off
      (30 downto 0)
      -- pragma translate_on
      );
  \c$ds_app_arg_2\ <= shift_left(\x'_0\,sh_7)
      -- pragma translate_off
      when (((to_signed(23,64) - n_0) and to_signed(2147483647,64)) >= 0) else (others => 'X')
      -- pragma translate_on
      ;
end block;

The problem looks to be the index 30 downto 0 but I don't exactly see why. That's a 31 bit part (of a 64-bit signal) written to a 'natural'. Is a natural 31 bits or 32 bits? I think it is 31 bit, but perhaps it is implemented as 32 in ghdl or vdl and something goes wrong writing 31 bits to it here via to_integer.

The 63 downto 31 part higher up can be questioned too, depending.

This I think is the generating pattern in Clash_Sized_Internal_BitVector.primitives.yaml:

    template: |-
      ~GENSYM[~RESULT_shiftR][0] : block
        signal ~GENSYM[sh][1] : natural;
      begin
        ~SYM[1] <=
            -- pragma translate_off
            natural'high when (~VAR[shI][2](~SIZE[~TYP[2]]-1 downto 31) /= 0) else
            -- pragma translate_on
            to_integer(~VAR[shI][2]
            -- pragma translate_off
            (30 downto 0)
           -- pragma translate_on
            );
        ~RESULT <= std_logic_vector(shift_right(unsigned(~ARG[1]),~SYM[1]))
            -- pragma translate_off
            when ((~ARG[2]) >= 0) else (others => 'X')
            -- pragma translate_on
            ;
      end block;

and in my old 1.5.0pre it used to be:

shift_left(~ARG[1],to_integer(~ARG[2]))

and that doesn't generate runtime errors in ghdl.

So, who is right? Clash or ghdl? Why does ghdl error here?

Regards

PTB

[martijnbastiaan]

I vaguely remember seeing this error before, and concluding it was a GHDL issue. Sadly I never got to the bottom of it. What version of GHDL are you running, and with which backend?

ARG ghdl_version="1.0.0" 
 RUN curl -L "https://github.com/ghdl/ghdl/archive/v$ghdl_version.tar.gz" \ 
         | tar -xz \ 
  && cd ghdl-$ghdl_version \ 
  && ./configure --with-llvm-config=llvm-config-11 --prefix=$PREFIX \ 
  && make -j$(nproc) \ 
  && make install

Here's what we're running on CI ^. I believe one of our test cases triggers the error if we upgrade GHDL / upgrade LLVM / switch to a different backend.

[pbreuer]

This is ghdl-gcc in debian unstable, which is

GHDL 1.0.0 (Debian 1.0.0+dfsg-8+b1) [Dunoon edition]
Compiled with GNAT Version: 10.3.0
GCC back-end code generator

I have an earlier v1.0.0 that I can try.

GHDL 1.0.0 (Debian 1.0.0+dfsg-1) [Dunoon edition]
Compiled with GNAT Version: 10.2.1 20210110
GCC back-end code generator

Are we sure it is not the to_integer(..) that is key? Is it OK at sending 31 bits to a natural (i.e. not an integer, exactly).

So, "oh, great, this could be a finger-pointing war between two sets of authors a to who is right about the standard". Those do not always progress. We know that the 31th bit is zero here, since you performed a test for the top 33 bits being zero, so how about just doing a (31 downto 0) instead of a (30 downto 0) to pass to the to_integer which pastes to a natural?

I think that was effectively how it was at the 1.5.0pre point, so to_integer might be happy with that. At the moment it looks to me that it is to_integer that is complaining. I don't know what I could vary it to in order to squeeze some more information out of the situation.

But I'll try. Anything to avoid reading the vhdl standard again in my life.

Thanks for the ideas. I'll do some more investigating.

PTB

[christiaanb]

I think Clash might be at fault here. So the Haskell type for shiftR# is:

BitVector n -> Int -> BitVector n

The Haskell types translate to the following VHDL types:

• BitVector n -> std_logic_vector(n-1 downto 0);
• Int -> signed(63 downto 0)

So the VHDL that we generate for shiftL# (without all the pragma translate stuff) is:

block
  signal shift_amount_n : natural
begin
  -- clip to highest natural when the shift amount either doesn't fit in 31 bits or is a negative number
  -- the reason we do this is so that shift_left doesn't throw a bound_check failure
  shift_amount_n <= natural'high when (shift_amount(63 downto 31) /= 0) else 
                    to_integer(shift_amount(30 downto 0));
  -- mimic Haskell behaviour by returning an error value when the shift amount is negative
  result <= shift_left(bv,shift_amount_n) when shift_amount >= 0 else
            (others => 'x');
end

But the issue is that:

to_integer(shift_amount(30 downto 0))

can return a negative integer when bit 30 is high, and the negative integer of course doesn't fit into a natural.

[christiaanb]

So then the solution would be to generate:

  shift_amount_n <= natural'high when (shift_amount(63 downto 31) /= 0) else
                    to_integer('0' & shift_amount(30 downto 0));

ensuring we end up with a positive 32-bit integer that we can easily translate into a natural.

[christiaanb]

Also, in case anyone is wondering. The reason we need to clip to 31 bit for natural numbers is that the spec says that:

subtype natural is integer range 0 to integer'high;

where integer'high according to the spec is minimally 2^31-1, and integer'low is -(2^31-1), i.e. the minimal integer range is the 32-bit 1's complement range. So the maximum integer range is implementation-dependent, but all implementations that I know make it the 32-bit 2's complement range.

[pbreuer]

Is to_integer('0' & shift_amount(30 downto 0)) the same as to_integer(shift_amount(31 downto 0)) in this branch of the conditional, since it is a branch in which the top 33 not 32 of 64 bits are zero?

Not as explicit. Shorter. About equally mysterious!

So the issue is that to_integer takes the top bit of its argument as a sign bit (which is unintended here) no matter how long it is, and preserves the sign into an integer of the right size (which is also unintended here as the target is unsigned).

Anyway, two solutions proposed and thank you very much!

PTB

[christiaanb]

Ah, yes, your solution makes more sense.

I guess we should generate a comment as to why we slice out the 32 bits like that.

[pbreuer]

One might also forget the truncation entirely and do to_integer(shift_amount) in this branch, since all the top 33 bits are known zero.

It would fail in a 16-bit context, that's all. The 0& business is invulnerable in all contexts.

[christiaanb]

One might also forget the truncation entirely and do to_integer(shift_amount) in this branch, since all the top 33 bits are known zero.

Indeed! good catch!

It would fail in a 16-bit context, that's all. The 0& business is invulnerable in all contexts.

Well... let's assume the simulators are at least spec compliant, meaning natural should at least fit [0..2^31), even if we were running on a 16-bit architecture.

[pbreuer]

I hope the branching would be still OK for a parallel vhdl simulation.

PTB

[martijnbastiaan]

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