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

rtlil: add Const::compress helper function #4608

Merged
merged 3 commits into from
Oct 13, 2024

Conversation

phsauter
Copy link
Contributor

@phsauter phsauter commented Sep 19, 2024

If one has a Sigspec that is fully constant it is currently non-trivial to get its value.
The main blocking point here is that the SigSpec may be wider than 32bit (an integer).
However, this doesn't mean the value stored in it is necessarily this large/small.

Eg something like 33'd4 or 33'd-2 cannot be easily converted to an int.

This adds a compress(is_signed) function.
It tries to minimize the number of bits used for the representation of the number.
It does so by removing the leading bits while taking signedness into consideration.

With this function, the above can be successfully converted to an integer by first calling compress.

Edit:
Here is a possible application.
https://github.com/povik/yosys-slang/blob/28cea56840a6bc8a4b6398d36609500d5960ad79/src/builder.cc#L134
With compress it is easy to check if the actual value has a larger representation than 24-bits instead of checking the SigSpec, which again may not use the minimum width.
I am fairly convinced there are places in the Yosys codebase itself this would be helpful, finding them is a bit difficult.

Copy link
Member

@povik povik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Some details need fixing

kernel/rtlil.cc Show resolved Hide resolved
kernel/rtlil.cc Outdated Show resolved Hide resolved
kernel/rtlil.cc Outdated
@@ -280,6 +280,32 @@ int RTLIL::Const::as_int(bool is_signed) const
return ret;
}

void RTLIL::Const::compress(bool is_signed)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation for not respecting/setting flags & CONST_FLAG_SIGNED ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we want to defer to that. That flag isn't set to a useful value in all contexts, especially when you convert a SigSpec with as_const(), which is the use case which prompted writing the helper.

@phsauter
Copy link
Contributor Author

Should I also add a .as_int_compress(bool is_signed) to make it even easier to get a constant integer?

@povik
Copy link
Member

povik commented Sep 23, 2024

For that one to be useful you need to know when you can trust the result and when not, so maybe it should return a pair of int, bool. If it can fail, then we may call it try_int.

@widlarizer
Copy link
Collaborator

That's what std::optional is for. What could fail?

@phsauter
Copy link
Contributor Author

If it is larger than 32-bit then it fails as it is not representable but yes, std::optional is a good option there.

@phsauter
Copy link
Contributor Author

Does this solution look reasonable?

@widlarizer
Copy link
Collaborator

As far as I can tell, it does

@widlarizer widlarizer added merge-after-jf Merge: PR will be merged after the next Dev JF unless concerns are raised and removed merge-after-jf Merge: PR will be merged after the next Dev JF unless concerns are raised labels Oct 13, 2024
@widlarizer widlarizer merged commit 61ed9b6 into YosysHQ:main Oct 13, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants