chipsalliance / sv-tests

Test suite designed to check compliance with the SystemVerilog standard.
https://chipsalliance.github.io/sv-tests-results/
ISC License
289 stars 75 forks source link

utd-sv_operators - incorrect unary operators #296

Closed drom closed 5 years ago

drom commented 5 years ago

Correct me if I am wrong, but UNARY (~& and ~|) operators were incorrectly used here:

https://symbiflow.github.io/sv-tests/#tree_sitter_verilog|utd-sv|utd-sv_operators (32:33)

   shifter = shifter ~& result;
   shifter = shifter ~| result;
Nic30 commented 5 years ago

Yes, they are incorrect, std. defines only unary version. It is Icarus extension and few other tools do support it as well.

tgorochowik commented 5 years ago

@piotr-binkowski maybe we could add some kind of a blacklisting mechanism for the third-party tests, and just skip tests that are invalid?

Nic30 commented 5 years ago

For some libraries there are such a blacklist in hdCovertor https://github.com/Nic30/hdlConvertor/blob/master/tests/test_verilator_testsuite.py#L59. (I do not know 100% if this the list is correct, I just checked the std. and tools on edaplayground.)

GitHub
Nic30/hdlConvertor
Fast Verilog/VHDL parser preprocessor and code generator for C++/Python based on ANTL4 - Nic30/hdlConvertor
drom commented 5 years ago

It is an imported test. Can we just fix it?

tgorochowik commented 5 years ago

@drom are the tests from 11.4.9 (e.g https://symbiflow.github.io/sv-tests/#tree_sitter_verilog|11.4.9|unary_op_nor ) not sufficient?

If you think so I guess we could add additional tests that mimic this one (but I guess with the should_fail flag?)

This is imported from an external test suite and I think that if they require fixing they should be fixed upstream first, but that might be hard if you take what @Nic30 said into account:

It is Icarus extension and few other tools do support it as well.

(i.e. the upstream may not want to get it fixed)

Nic30 commented 5 years ago

I can confirm that the upstream usually do not want to "get it fixed". I found only one discussion about this https://github.com/steveicarus/ivtest/issues/8#issuecomment-527216222 but they were all the same.

drom commented 5 years ago

@tgorochowik I agree that generated 11.4.9 tests should be sufficient, and we should mark this test as should_fail = 1 .

mithro commented 5 years ago

Maybe we need a "vendor extension" type tag? If this is actually found in real code in the wild we should probably check for support...

Nic30 commented 5 years ago

I tried to build a list of such an extensions, however how it seems to me that the "extension" is just different term for "wont fix".