Make compile for Yosys 0.37

[hzeller]

Yosys added another parameter to AST::process(), which we have to track.

Unfortunately, there is no easy way to #ifdef on a yosys version that I know of (there do not seem to be macros exposed), so this will not compile with old versions anymore.

Fixes #2299

[hzeller]

Of course, the build will now fail as it assumes Yosys 0.36.
If there is a way to detect the Yosys version with a macro, please advise, then I can make it work for both versions.
If not easily, then I suggest to just roll forward to use 0.37

[pgielda]

Yosys version seems to be a variable in its Makefile: https://github.com/YosysHQ/yosys/blob/master/Makefile#L144
We could potentially try to somehow leverage that

[pgielda]

FYI @tgorochowik @kgugala

[tgorochowik]

@hzeller at this point I think it might be worth considering analyzing the yosys binary we're building with, extracting the mangled symbol and calling it with dlsym

I pushed a simple PoC here:
8c4e1ee

The implementation above works on my machine with both old and new yosys, but it's obviously not universal, because symbol mangling might differ between platforms, but the symbols could be checked with nm in Makefile and propagated through a define if we go with a similar idea.

Another thought I have is using a weak symbol with all arguments that would call the original symbol with fewer arguments (and a weak empty symbol without the extra argument so it still compiles with new yosys), but I'm not sure this can be simply pushed to an external namespace, might be worth an experiment.

[alaindargelas]

@hzeller , please update yosys in this PR too. It currently breaks as you are still using the previous yosys.
PS, I would not worry about backward compatibility.

[tgorochowik]

PS, I would not worry about backward compatibility.

Good point, in general I agree, however I think latest changes in yosys (before the API change) broke some regression tests in the plugin. These might need to be fixed before we can bump yosys

[alaindargelas]

@hzeller all the failures probably require porting all the changes happening in:
YosysHQ/yosys@ea7818d...80511ce#diff-9cf03955fa6ed7051b83cfb95de9e3c42eeeecdff3fa071fbb14ac43e41b12a0
Open simplify.cc

To:

./third_party/yosys_mod/synlig_simplify.cc

BTW, this ./third_party/yosys_mod/synlig_simplify.cc is going to be an eternal problem. Check the existing diffs even before this new PR. Maintenance nightmare...

tkdiff ./third_party/yosys/frontends/ast/simplify.cc ./third_party/yosys_mod/synlig_simplify.cc

Bringing those changes in, might in turn require some minor changes in the rest of the plugin.

I think that makes the point about trying to maintain backward compatibility based one function signature moot.

[hzeller]

Yeah, it looks like we can only strictly move forward with only marginal backward compatibility.

I have not tracked that, how is synlig_simplify.cc different from the origin it was coming from ? Unfortunately, all version information is lost that it would be possible to see what is different.
Moving forward, it would probably be a good idea to also add the commit hash a synlig_simplify.cc was derived from yosys.

[hzeller]

These were the last relevant changes in the synlig_simplify.cc
f97999d
c5bd735

So change in simplify.cc in yosys are

git diff -uw -r 8367f06188edf750b32bd603552ba9d75995baf5 -r 80511ced71b657e77a5389d54c6aa045c047ccaf frontends/ast/simplify.cc

...

[alaindargelas]

The problem is much larger than that.
This file is essentially a hard fork of the Yosys simplify.cc,
here is an example of diff, the 2 bodies of the same function have diverged considerably.

To checkout the repo before the file name change:

git checkout 01111c5
git submodule update --recursive
Then you can perform the diff.

The Antmicro guys had a methodology to merge-in changes as the one your pointed out above. Please check with them if they can help.
It's a 3-way diff merge that has to happen essentially.

[tgorochowik]

It's possible that it's a matter of backporting the changes from simplify. It's not possible to say for certain without an actual analysis of the new errors (or without actually doing this). I still think having a slim compatibility layer to not have to deal with that each time API changes could be beneficial (of course I agree it'd be better to just fix those issues, but having sth to unblock you until this is done would be nice).

@hzeller since you didn't like the dlsym approach, PTAL at a simple workaround with the use of weak linking: dc75266 - it works for me both with old and new yosys. Of course it has it's own limitations (i.e. not all platforms support weak linking), if something similar breaks in the future adding extra wrappers similar to this one should be super quick.

[hzeller]

I think I like the weak-linkage approach in
dc75266 better @tgorochowik

Can you format it neatly like I did in this PR with comments on what these values are ? It is super-confusing and easy to miss what the n'th boolean means :)

[kamilrakoczy]

Solution with compatibility layer was merged in: #2357
and now we merged PR that is updating yosys 0.44 version (current upstream): #2510

This PR isn't needed anymore, closing.