New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Index maxBound #79

Merged
merged 4 commits into from Sep 18, 2015

Conversation

Projects
None yet
3 participants
@leonschoorl
Contributor

leonschoorl commented Sep 16, 2015

This started out as one simple thing, and then became many things.
If you like I can open separate issues.

First the vhdl primitive for maxBound on Index was broken.
Here is a fix for that.

I also included a test which checks the bounds of Index and some other types.

But this test currently fails, because it turns out that when min/maxBound of type Unsigned are converted to Integer, then something breaks.

When translating clash seems think:

toInteger (minBound :: Unsigned 9) == 9
toInteger (maxBound :: Unsigned 9) == 2^9 -- should be 2^9-1

Furthermore the testbench for Bounds should currently fail for both VHDL and Verilog.
And it does fail in both Modelsim and ISim.
But I couldn't get iverilog/vvp to fail it on my machine.
If the vvp test for this succeeds on the Travis CI system in the current state, then is a bug.

@leonschoorl

This comment has been minimized.

Show comment
Hide comment
@leonschoorl

leonschoorl Sep 16, 2015

Contributor

The test also succeeded on the Travis system, so it's not just my system.

I did some further digging.
When you modify the generated Verilog like this:

--- Bounds_outputVerifierzm_11.v.orig   2015-09-16 17:07:55.743103623 +0200
+++ Bounds_outputVerifierzm_11.v    2015-09-16 17:14:42.189103247 +0200
@@ -43,6 +43,10 @@
     if (eta_i2 != repANF_0) begin
       $display("%s, expected: %b, actual: %b", ("outputVerifier"), repANF_0, eta_i2);
       $finish;
+    end else if (eta_i2 == repANF_0) begin
+      $display("%s, expected: %b, considered equal to actual: %b", ("outputVerifier"), repANF_0, eta_i2);
+    end else begin
+      $display("%s, expected: %b, considered neither equal nor unequal to actual: %b", ("outputVerifier"), repANF_0, eta_i2);
     end
   end
   // pragma translate_on

It shows what's happening in the test:

outputVerifier, expected: 00000000000000000000000000000000, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 00000000000000000000000000000000, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 00000000000000000000000000000000, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 11111111111111111111111100000000, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 00000000000000000000000011111111, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 00000000000000000000000000000000, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 00000000000000000000000111111111, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 11111111111111111111111111000000, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 00000000000000000000000000111111, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 00000000000000000000000000000000, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 00000000000000000000000001111111, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz

Now the question is why it's getting those z's

Contributor

leonschoorl commented Sep 16, 2015

The test also succeeded on the Travis system, so it's not just my system.

I did some further digging.
When you modify the generated Verilog like this:

--- Bounds_outputVerifierzm_11.v.orig   2015-09-16 17:07:55.743103623 +0200
+++ Bounds_outputVerifierzm_11.v    2015-09-16 17:14:42.189103247 +0200
@@ -43,6 +43,10 @@
     if (eta_i2 != repANF_0) begin
       $display("%s, expected: %b, actual: %b", ("outputVerifier"), repANF_0, eta_i2);
       $finish;
+    end else if (eta_i2 == repANF_0) begin
+      $display("%s, expected: %b, considered equal to actual: %b", ("outputVerifier"), repANF_0, eta_i2);
+    end else begin
+      $display("%s, expected: %b, considered neither equal nor unequal to actual: %b", ("outputVerifier"), repANF_0, eta_i2);
     end
   end
   // pragma translate_on

It shows what's happening in the test:

outputVerifier, expected: 00000000000000000000000000000000, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 00000000000000000000000000000000, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 00000000000000000000000000000000, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 11111111111111111111111100000000, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 00000000000000000000000011111111, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 00000000000000000000000000000000, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 00000000000000000000000111111111, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 11111111111111111111111111000000, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 00000000000000000000000000111111, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 00000000000000000000000000000000, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
outputVerifier, expected: 00000000000000000000000001111111, considered neither equal nor unequal to actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz

Now the question is why it's getting those z's

@christiaanb

This comment has been minimized.

Show comment
Hide comment
@christiaanb

christiaanb Sep 16, 2015

Contributor

c409e2b fixes the static evaluator. Can rebase your branch on top of it?

Contributor

christiaanb commented Sep 16, 2015

c409e2b fixes the static evaluator. Can rebase your branch on top of it?

@christiaanb

This comment has been minimized.

Show comment
Hide comment
@christiaanb

christiaanb Sep 17, 2015

Contributor

b8b7d6a fixes the all z's problem in the verilog testbench

Contributor

christiaanb commented Sep 17, 2015

b8b7d6a fixes the all z's problem in the verilog testbench

@leonschoorl

This comment has been minimized.

Show comment
Hide comment
@leonschoorl

leonschoorl Sep 17, 2015

Contributor

Yes, it will rebase it on the current master, tomorrow.
Let me just have Travis check this in the meantime.

Contributor

leonschoorl commented Sep 17, 2015

Yes, it will rebase it on the current master, tomorrow.
Let me just have Travis check this in the meantime.

@leonschoorl

This comment has been minimized.

Show comment
Hide comment
@leonschoorl

leonschoorl Sep 18, 2015

Contributor

Mmm, with that it fails with

outputVerifier, expected: 00000000000000000000000000000000, actual: 00000000000000000010101000101010

Where as on my local machine this fails with

outputVerifier, expected: 00000000000000000000000000000000, actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz

Seems like Travis is automatically merging this with master before checking, and the z's are gone.

Contributor

leonschoorl commented Sep 18, 2015

Mmm, with that it fails with

outputVerifier, expected: 00000000000000000000000000000000, actual: 00000000000000000010101000101010

Where as on my local machine this fails with

outputVerifier, expected: 00000000000000000000000000000000, actual: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz

Seems like Travis is automatically merging this with master before checking, and the z's are gone.

leonschoorl added some commits Sep 16, 2015

Added a test which checks the min/maxBound of various CLaSH types
I tried to pick some non-trivial case, ie non powers of 2.

Note that this test currently fails, because:
	- maxBound on Index is broken for VHDL
	- min/maxBound on Unsigned are broken when being converted to Integer

The maxBound on Index will be fixed in the next commit.
Have the (system)verilog testbench realy check the values
Even if the the bits are X,Z,etc.

This should prevent any false positives in the tests results.
@leonschoorl

This comment has been minimized.

Show comment
Hide comment
@leonschoorl

leonschoorl Sep 18, 2015

Contributor

I traced the source of the ghdl error: bound check failed (#11)
In both ...stimuliGeneratorzm... and ...outputVerifierzm... it generates something like this:

signal repANF_8  : unsigned(3 downto 0);
signal repANF_11 : integer;
...
repANF_8 <= to_unsigned(repANF_11,integer(ceil(log2(real(max(2,10))))));
repANF_11 <= 10 - 1;

And ghdl doesn't like that conversion to_unsigned of the signal, even though the signal is static.
I don't know what the VHDL standard says about this, but ISim has no problems with it.

Contributor

leonschoorl commented Sep 18, 2015

I traced the source of the ghdl error: bound check failed (#11)
In both ...stimuliGeneratorzm... and ...outputVerifierzm... it generates something like this:

signal repANF_8  : unsigned(3 downto 0);
signal repANF_11 : integer;
...
repANF_8 <= to_unsigned(repANF_11,integer(ceil(log2(real(max(2,10))))));
repANF_11 <= 10 - 1;

And ghdl doesn't like that conversion to_unsigned of the signal, even though the signal is static.
I don't know what the VHDL standard says about this, but ISim has no problems with it.

@ggreif

This comment has been minimized.

Show comment
Hide comment
@ggreif

ggreif Sep 18, 2015

Contributor

@leonschoorl did you try to report this as a ghdl bug?

Contributor

ggreif commented Sep 18, 2015

@leonschoorl did you try to report this as a ghdl bug?

@leonschoorl

This comment has been minimized.

Show comment
Hide comment
@leonschoorl

leonschoorl Sep 18, 2015

Contributor

No, I just figured this out.
And I don't know that much about vhdl, for all I know ghdl might actually be doing the "right thing" according to the standard.

Although is it at least annoying that it doesn't provide any location information for the error.

Contributor

leonschoorl commented Sep 18, 2015

No, I just figured this out.
And I don't know that much about vhdl, for all I know ghdl might actually be doing the "right thing" according to the standard.

Although is it at least annoying that it doesn't provide any location information for the error.

@christiaanb

This comment has been minimized.

Show comment
Hide comment
@christiaanb

christiaanb Sep 18, 2015

Contributor

The problem is that, initially, all scalars in VHDL are initialised to their so-called 'left value. repANF_11 is of VHDL type integer, and the Integer'left value is (-2^31)-1.

The type of to_unsigned is, according to the VHDL standard (using haskell notation):

to_unsigned :: natural -> natural -> unsigned

where natural is defined as:

subtype natural is integer range 0 to integer'right

That is, natural is subtype of integer, i.e. it is "type-safe" to assign integers to naturals, and natural to integers.

What you see is that, at initialisation of the VHDL simulator, 0ns delta cycle 0, repANF_11, has the value (-2^31)-1. In the next delta cycle, repANF_11 becomes 9, however, in the same delta cycle, the to_unsigned function is already applied to the old value of repANF_11, which is outside of the range of the natural type. That is why you get the bound check failure.

Perhaps other simulators either initialise integer signals to a different value, e.g. 0, or they ignore bound-check failures for scalar assignments for the first few delta cycles.

Contributor

christiaanb commented Sep 18, 2015

The problem is that, initially, all scalars in VHDL are initialised to their so-called 'left value. repANF_11 is of VHDL type integer, and the Integer'left value is (-2^31)-1.

The type of to_unsigned is, according to the VHDL standard (using haskell notation):

to_unsigned :: natural -> natural -> unsigned

where natural is defined as:

subtype natural is integer range 0 to integer'right

That is, natural is subtype of integer, i.e. it is "type-safe" to assign integers to naturals, and natural to integers.

What you see is that, at initialisation of the VHDL simulator, 0ns delta cycle 0, repANF_11, has the value (-2^31)-1. In the next delta cycle, repANF_11 becomes 9, however, in the same delta cycle, the to_unsigned function is already applied to the old value of repANF_11, which is outside of the range of the natural type. That is why you get the bound check failure.

Perhaps other simulators either initialise integer signals to a different value, e.g. 0, or they ignore bound-check failures for scalar assignments for the first few delta cycles.

@christiaanb

This comment has been minimized.

Show comment
Hide comment
@christiaanb

christiaanb Sep 18, 2015

Contributor

As a work-around, make the expected and actual vectors their own separate vector literals:

expected = 0 :> 42 :> ... :> Nil
actual = (minBound :: Index 43) :> (maxBound :: Index 43) :> ... :> Nil
Contributor

christiaanb commented Sep 18, 2015

As a work-around, make the expected and actual vectors their own separate vector literals:

expected = 0 :> 42 :> ... :> Nil
actual = (minBound :: Index 43) :> (maxBound :: Index 43) :> ... :> Nil
@leonschoorl

This comment has been minimized.

Show comment
Hide comment
@leonschoorl

leonschoorl Sep 18, 2015

Contributor

That works for the case in outputVerifierzm, but because of the way I generate the stimuli
stimuli = map (const ()) actual
There is still the same problem in the stimuliGeneratorzm.

I would like to do
stimuli = replicate (length actual) ()

But it doesn't seem possible to get the length of a vector as SNat.

Contributor

leonschoorl commented Sep 18, 2015

That works for the case in outputVerifierzm, but because of the way I generate the stimuli
stimuli = map (const ()) actual
There is still the same problem in the stimuliGeneratorzm.

I would like to do
stimuli = replicate (length actual) ()

But it doesn't seem possible to get the length of a vector as SNat.

@christiaanb

This comment has been minimized.

Show comment
Hide comment
@christiaanb

christiaanb Sep 18, 2015

Contributor

just do:

testInput :: Signal ()
testInput = signal ()
Contributor

christiaanb commented Sep 18, 2015

just do:

testInput :: Signal ()
testInput = signal ()
Separate the vectors in the Bounds test
This makes them easier to translate from clash
@leonschoorl

This comment has been minimized.

Show comment
Hide comment
@leonschoorl

leonschoorl Sep 18, 2015

Contributor

Oh that's easy.

I was under the impression the there was something "magic" about stimuliGenerator that was required to generate a proper testbench.

And another thing I ran into here, pattern matching on :> doesn't work.
Is that bug?

Contributor

leonschoorl commented Sep 18, 2015

Oh that's easy.

I was under the impression the there was something "magic" about stimuliGenerator that was required to generate a proper testbench.

And another thing I ran into here, pattern matching on :> doesn't work.
Is that bug?

@christiaanb

This comment has been minimized.

Show comment
Hide comment
@christiaanb

christiaanb Sep 18, 2015

Contributor

No, there's nothing "magic" about stimuliGenerator, any function producing a Signal a will do.

With regards to pattern matching on :>: it's a known unsupported haskell feature: see GADT pattern matching.

I'm thinking to work around it by introducing a pattern synonym in the next version of the CLaSH prelude.

Contributor

christiaanb commented Sep 18, 2015

No, there's nothing "magic" about stimuliGenerator, any function producing a Signal a will do.

With regards to pattern matching on :>: it's a known unsupported haskell feature: see GADT pattern matching.

I'm thinking to work around it by introducing a pattern synonym in the next version of the CLaSH prelude.

christiaanb added a commit that referenced this pull request Sep 18, 2015

@christiaanb christiaanb merged commit 11c6bbb into clash-lang:master Sep 18, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@leonschoorl leonschoorl deleted the leonschoorl:fix-index-maxbound branch Nov 6, 2015

christiaanb added a commit that referenced this pull request Sep 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment