On 5/02/11 5:48 AM, Tristan Gingold wrote:
>
> ----- "Wesley J. Landaker" <[email protected]> a écrit :
>
>> Hey Tristan,
>>
>> I found an interesting (and annoying) bug in GHDL's handling of
>> bit_vector rol
>> and ror. Basically, rol and ror work fine, except when the amount to
>> rotate
>> exactly matches the length of the bit_vector, in which case it gets
>> rotated
>> incorrectly by one place.
>
> Interesting bug...
>
> Tristan.
For the rest of us:
The bug is a bit more subtle than the above description. The vhdl file used
is the second version with proper report statements for rol and ror.
bit_vector_rol_ror_bug.vhdl:20:3:@0ms:(assertion warning): ror 5 is broken
assert ((bit_vector'("11100") ror 5) = "11100") report "ror 5 is broken"
severity warning;
bit_vector_rol_ror_bug.vhdl:28:3:@0ms:(assertion warning): rol -5 is broken
assert ((bit_vector'("11100") rol -5) = "11100") report "rol -5 is broken"
severity warning;
Note that ror -5 and rol 5 passed.
Modifying bit_vector_rol_ror_bug.vhdl, the two lines generating an
assertion, by adding function TO_VECTOR to the architecture declarative
region and outputting the input value and the ror/rol result:
../bit_vector_rol_ror_bug
bit_vector_rol_ror_bug.vhdl:41:3:@0ms:(assertion warning): ror 5 is broken
11100 produces 00000!
bit_vector_rol_ror_bug.vhdl:49:3:@0ms:(assertion warning): rol -5 is broken
11100 produces 00000!
(Anyone know where the severity clause went? (LRM 93 8.2) Looks like a
parser error? It looks like someone is looking a token ahead (assuming a
string is a token) - this is executed on ghdl-0.28, see below why that's
okay for the original bug. I tried enclosing the whole mess following
report up to severity as a cast string in parens and it didn't help. Back to
the bug at hand:)
Outputting both the input and the output was to insure TO_VECTOR worked
correctly. See the appended marked up bit_vector_rol_ror_bug.vhdl file.
If I recall correctly in ghdl's evaluation.adb:716 function Eval_Shift_Operator
when Iir_Predefined_Array_Rol
| Iir_Predefined_Array_Ror =>
for I in 1 .. Len loop
Append_Element
(Res_List, Get_Nth_Element (Arr_List, Cnt));
Cnt := Cnt + 1;
if Cnt = Len then
Cnt := 0;
end if;
end loop;
end case;
return Build_Simple_Aggregate (Res_List, Origin, Get_Type (Left));
end Eval_Shift_Operator;
the function Get_Nth_Element will return a null for an out of range index:
lists.adb:68
function Get_Nth_Element (List: List_Type; N: Natural)
return Node_Type
is
begin
if N >= Listt.Table (List).Nbr then
return Null_Node;
end if;
return Listt.Table (List).Els (N);
end Get_Nth_Element;
I don't understand gnat lists but it looks like Append_Element isn't adding
any of the three '1' values from any position in Arr_List to Res_List
because Cnt isn't a valid element index for Arr_List for these two cases,
for at least the three '1' bits.
(And in case you can't tell I'm trying not to debug ghdl).
It would seem the problem lies around
evaluation.adb:653
if Count < 0 then
Cnt := Natural (-Count);
Dir_Left := not Dir_Left;
else
Cnt := Natural (Count);
end if;
or evaluation.adb:677
when Iir_Predefined_Array_Rol
| Iir_Predefined_Array_Ror =>
Cnt := Cnt mod Len;
if not Dir_Left then
Cnt := Len - Cnt;
end if;
end case;
(sorry, no highlighting in text email).
My guess is around the second of these two code segments. Dir_Left is given
as the absolute shift direction for the array. The ror 5 failed case will
have Dir_Left false initially, and Count = 5, while the rol -5 failed case
will have Dir_Left True initially with Count = -5(not shown in the code
segments above, the case statement immediately preceding line 653).
In segment 653 Cnt will be assigned 5 for both ror 5 and rol -5. Both cases
will end up with Dir_Left False. Len should be 5 for "11100". Assuming Cnt
:= Cnt mod Len results in 0, because both are Dir_Left False they'll both
end up with 5 (Len - 0 = 5).
back to evaluation.adb:716
when Iir_Predefined_Array_Rol
| Iir_Predefined_Array_Ror =>
for I in 1 .. Len loop
Append_Element
(Res_List, Get_Nth_Element (Arr_List, Cnt));
Cnt := Cnt + 1;
if Cnt = Len then
Cnt := 0;
end if;
end loop;
end case;
The if Cnt = Len doesn't get invoked when Cnt is already equal to Len and
then gets incremented before testing. Cnt is staying out of range not being
a valid Nbr element in the List.
Back to evaluation.adb:677
when Iir_Predefined_Array_Rol
| Iir_Predefined_Array_Ror =>
Cnt := Cnt mod Len;
if not Dir_Left then
Cnt := Len - Cnt;
end if;
end case;
Dir_Left is carrying the direction of shift. It's used to determine the
intial offset in Arr_List copying at evaluation.adb:716 which is copied from
left to right modulo Len. Remember both our error cases have Dir_Left False
The error case we are dealing with is Cnt = Len following the above if
statement, because Cnt is being incremented before testing against Len.
valuation.adb:680 gets modified to look something like this:
if Cnt /= 0 and not Dir_Left then
Cnt := Len - Cnt;
end if;
If there is no shift (Cnt = 0) there is no offset needed.
I had to sleep on this one. I had the answer yesterday without recognizing
it. Both a performance improvement and the fix can be implemented here by:
evaluation.adb:677
when Iir_Predefined_Array_Rol
| Iir_Predefined_Array_Ror =>
Cnt := Cnt mod Len;
if Cnt = 0 then
return Build_Simple_Aggregate (Arr_List, Origin, Get_Type
(Left));
end if;
if not Dir_Left then
Cnt := Len - Cnt;
end if;
end case;
Which saves a little code traversal, too. The idea being if Cnt = 0 no
offset copying is required, you can return the original array and short
circuit further copying (Arr_List was already produced from Left).
The rest was remembering where to find a TO_STRING function I could use and
digging out an Ada LRM to use. The idea for the former to understand
exactly what was going on. TO_STRING in the report specification of an
assertion string tells us the result wasn't off by a shift index.
A caution the actual execution was on a ghdl-0.28 I compiled a year or two
ago, but the only differences in evaluation.adb I found were in function
Get_Physical_Value and the Len type declaration in function
Eval_Dyadic_Bit_Array_Operator looking through my svn logs and comparing the
two version. (There's also comment changes and some minor white space
differences).
The missing severity clause for another time. I may have to break down and
get 0.29 working.
library ieee;
use ieee.std_logic_1164.all;
entity bit_vector_rol_ror_bug is
end entity;
architecture ghdl_bug of bit_vector_rol_ror_bug is
function TO_STRING (VALUE : BIT_VECTOR) return STRING is
alias ivalue : BIT_VECTOR(1 to value'length) is value;
variable result : STRING(1 to value'length);
begin
if value'length < 1 then
return "";
else
for i in ivalue'range loop
if iValue(i) = '0' then
result(i) := '0';
else
result(i) := '1';
end if;
end loop;
return result;
end if;
end function to_string;
begin
assert ((bit_vector'("11100") ror -8) = "00111") report "ror -8 is broken"
severity warning;
assert ((bit_vector'("11100") ror -7) = "10011") report "ror -7 is broken"
severity warning;
assert ((bit_vector'("11100") ror -6) = "11001") report "ror -6 is broken"
severity warning;
assert ((bit_vector'("11100") ror -5) = "11100") report "ror -5 is broken"
severity warning;
assert ((bit_vector'("11100") ror -4) = "01110") report "ror -4 is broken"
severity warning;
assert ((bit_vector'("11100") ror -3) = "00111") report "ror -3 is broken"
severity warning;
assert ((bit_vector'("11100") ror -2) = "10011") report "ror -2 is broken"
severity warning;
assert ((bit_vector'("11100") ror -1) = "11001") report "ror -1 is broken"
severity warning;
assert ((bit_vector'("11100") ror 0) = "11100") report "ror 0 is broken"
severity warning;
assert ((bit_vector'("11100") ror 1) = "01110") report "ror 1 is broken"
severity warning;
assert ((bit_vector'("11100") ror 2) = "00111") report "ror 2 is broken"
severity warning;
assert ((bit_vector'("11100") ror 3) = "10011") report "ror 3 is broken"
severity warning;
assert ((bit_vector'("11100") ror 4) = "11001") report "ror 4 is broken"
severity warning;
assert ((bit_vector'("11100") ror 5) = "11100") report string'("ror 5 is
broken " & TO_STRING(bit_vector'("11100"))&" produces "&
TO_STRING(bit_vector'(("11100") ror 5)) &"!") severity warning;
assert ((bit_vector'("11100") ror 6) = "01110") report "ror 6 is broken"
severity warning;
assert ((bit_vector'("11100") ror 7) = "00111") report "ror 7 is broken"
severity warning;
assert ((bit_vector'("11100") ror 8) = "10011") report "ror 8 is broken"
severity warning;
assert ((bit_vector'("11100") rol -8) = "10011") report "rol -8 is broken"
severity warning;
assert ((bit_vector'("11100") rol -7) = "00111") report "rol -7 is broken"
severity warning;
assert ((bit_vector'("11100") rol -6) = "01110") report "rol -6 is broken"
severity warning;
assert ((bit_vector'("11100") rol -5) = "11100") report "rol -5 is broken " &
TO_STRING(bit_vector'("11100"))&" produces "& TO_STRING(bit_vector'(("11100")
ror 5)) &"!" severity warning;
assert ((bit_vector'("11100") rol -4) = "11001") report "rol -4 is broken"
severity warning;
assert ((bit_vector'("11100") rol -3) = "10011") report "rol -3 is broken"
severity warning;
assert ((bit_vector'("11100") rol -2) = "00111") report "rol -2 is broken"
severity warning;
assert ((bit_vector'("11100") rol -1) = "01110") report "rol -1 is broken"
severity warning;
assert ((bit_vector'("11100") rol 0) = "11100") report "rol 0 is broken"
severity warning;
assert ((bit_vector'("11100") rol 1) = "11001") report "rol 1 is broken"
severity warning;
assert ((bit_vector'("11100") rol 2) = "10011") report "rol 2 is broken"
severity warning;
assert ((bit_vector'("11100") rol 3) = "00111") report "rol 3 is broken"
severity warning;
assert ((bit_vector'("11100") rol 4) = "01110") report "rol 4 is broken"
severity warning;
assert ((bit_vector'("11100") rol 5) = "11100") report "rol 5 is broken"
severity warning;
assert ((bit_vector'("11100") rol 6) = "11001") report "rol 6 is broken"
severity warning;
assert ((bit_vector'("11100") rol 7) = "10011") report "rol 7 is broken"
severity warning;
assert ((bit_vector'("11100") rol 8) = "00111") report "rol 8 is broken"
severity warning;
end architecture;_______________________________________________
Ghdl-discuss mailing list
[email protected]
https://mail.gna.org/listinfo/ghdl-discuss
