Skip to content
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

Clash 1.6.3 generates vhdl with undeclared "and" errors #2213

Closed
pbreuer opened this issue May 18, 2022 · 6 comments
Closed

Clash 1.6.3 generates vhdl with undeclared "and" errors #2213

pbreuer opened this issue May 18, 2022 · 6 comments
Labels

Comments

@pbreuer
Copy link

pbreuer commented May 18, 2022

The error from ghdl is:

ghdl-gcc -a Test_Trace_kpu_test32_raw_tlbs_unit.vhdl
Test_Trace_kpu_test32_raw_tlbs_unit.vhdl:27370:19: no function declarations for operator "and"
... (repeat many times)...

This is an error, not a warning, unfortunately. It appears to arise only from an "and" produced in a block where "pragma translate off" holds, but that is not the new element, it must be something missing somewhere else that is merely exposed by that.

The above error is from this segment of generated vhdl, and note there is an "and" just above that causes no error:

\c$karg_app_arg\ <= std_logic_vector(rotate_left(unsigned(result_42),to_integer((y1_54 and to_signed(2147483647,64))
-- pragma translate_off
mod 473
-- pragma translate_on
)))
-- pragma translate_off
when (y1_54 and to_signed(2147483647,64) >= 0) else (others => 'X')
-- pragma translate_on
;

The difference between the two ands there appears to be the pragma translate business, and that also appears to be the difference in general between those ands that trigger errors and those that do not, though I have not checked thoroughly.

It is not specific to "and". I changed the "and" to an "or" and got the same error, but referencing "or":

Test_Trace_kpu_test32_raw_tlbs_unit.vhdl:27370:19: no function declarations for operator "or"

The reason I say the error appears to be a lack elsewhere is that the vhdl generated by my Clash 1.5.0pre installation causes no such error from ghdl, although it has the same form, to within an extra "mod 473":

\c$karg_app_arg\ <= std_logic_vector(rotate_left(unsigned(result_43),to_intege
r(((y1_54 and to_signed(2147483647,64))))))
-- pragma translate_off
when (((y1_54 and to_signed(2147483647,64))) >= 0) else (others => 'X')
-- pragma translate_on
;

No error from ghdl.

I have removed the extra "mod 473" and surrounding translate off/on in the modern code, but the error persists. So .. something missing somewhere else, both "and" and "or", only visible in parts where translation is off.

Does that cause any "aha!"s? The includes appear the same between good and bad codes.

Regards

PTB

@christiaanb
Copy link
Member

I don't know the root cause of Clash generating fewer parenthesis, but it seems 6adeb73 didn't go far enough in adding parenthesis for the rotate blackbox code.

@pbreuer
Copy link
Author

pbreuer commented May 18, 2022

Confirm. An extra parenthesis pair around the and conjunct before the ">=" makes the error disappear.

Thanks for the quick diagnosis!

PTB

@pbreuer
Copy link
Author

pbreuer commented May 19, 2022

Neither the template for rotateL nor and have changed in that respect since 1.5.0pre. It's "when (~ARG[2] >= 0) else (others => 'X')" and "~ARG[1] and ~ARG[2]" in both. Has the interpretation of ~ARG[N] changed so there are now no parentheses automatically added?

It would be a generic problem either way. One needs to protect compound expressions with weak binding when they appear in a context where there is strong(er) binding. Which way to go about it is a SE strategy question.

Not many of the templates in prims/vhdl/Clash_Sized_Internal_*.primitives.yaml look safe to me with respect to possible operator binding captures into ~ARG[N].

Peter

leonschoorl added a commit that referenced this issue Jun 13, 2022
This makes sure there are no unexpected operator precedence issues
between operators in the blackbox and operators in the expression of the
~ARG.

This might cause a few superfluous parentheses somewhere.
But it fixes a lot of missing cases, like discovered in #2213.
And clash is smart enough to leave them out when the ~ARG is
simple enough, like a literal or a variable.

Fixes #2213
@leonschoorl
Copy link
Member

There used to be an invisible blackbox for GHC.Types.I#, the data constructor for Int.
The existence of that blackbox coincidentally caused clash to split the and out into a separate signal.
That blackbox was removed in #1972, and that's why the behavior changed.

And you're right, there are many more primitives that are potentially vulnerable to this problem.
I'm changing clash so it'll automatically put parentheses around ~ARG[n].

mergify bot pushed a commit that referenced this issue Jun 17, 2022
This makes sure there are no unexpected operator precedence issues
between operators in the blackbox and operators in the expression of the
~ARG.

This might cause a few superfluous parentheses somewhere.
But it fixes a lot of missing cases, like discovered in #2213.
And clash is smart enough to leave them out when the ~ARG is
simple enough, like a literal or a variable.

Fixes #2213

(cherry picked from commit 88ef335)
@pbreuer
Copy link
Author

pbreuer commented Jul 15, 2022

Just a late note. I have now generated Verilog from the same Clash source and "it doesn't work". Vhdl is running correctly under ghdl (I have been porting your updates).

It compiles and runs in Icarus Verilog, but the behavior is wrong. Too subtle to deduce exactly what, but it appears to be cumulative so might be fundamental. Things go wrong about cycle 400. Did you check the parenthesis and precedence issue for verilog as well as vhdl?

The generated verilog does not compile at all under verilator (to c++). No error but it eventually runs out of memory on compiling the verilog source files larger than about 40KB. That's in about 128GB of ram+swap. Verilator seems to pre-take what memory it can get and step on it, so I am running without memory_overcommit in the kernel, otherwise things go bad faster.

What can I try the system verilog under?Iverilog also?

Regards

PTB

@martijnbastiaan
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants