[Moore] Add shortreal type, bit <-> real conversions
#8985
Conversation
Scheremo
force-pushed
the
pr-b2r
branch
4 times, most recently
from
336e666 to
81d0fdf
Compare
This PR disambiguates the RealType into 32-Bit real types ("shortreal") and 64-Bit real types ("real").
It adds support for materializing moore.shortreal from shortreal-specific statements like
localparam shortreal sr = 4.532;
as well.
Apart from this new type disctinction, this PR also adds support for the builtins $shortrealtobits, $bitstoshortreal, $realtobits and $bitstoreal.
When working through the builtins mentioned above, I noticed that the shortreal type was missing.
I decided to add it as !moore.shortreal since the distinction between $shortrealtobits and $realtobits would not really make sense in the previous type system.
To keep backwards compatibility (and also because most real values in Verilog are actually real, not shortreal), I chose to default the new width parameter of RealType to 64 and add a TypeBuilder that accepts "widthless" construction and defaults to 64 Bit.
While Section 20.5 "Conversion functions" is unspecific about the domain of the "bits" end of the conversion functions, I infer these two lines to mean two-valued bit rather than logic vectors only:
$realtobits converts values from a real type to a 64-bit vector representation of the real number.
$bitstoreal converts a bit pattern created by $realtobits to a value of the real type.
fabianschuiki
left a comment
fabianschuiki
left a comment
There was a problem hiding this comment.
Very cool! Just a few nits here and there 😁
lib/Dialect/Moore/MooreTypes.cpp
Outdated
| void RealType::print(mlir::AsmPrinter &p) const { | ||
| // Print bare `real` when default (64), otherwise `real<32>` | ||
| if (getWidth() == 64) | ||
| return; | ||
| p << "<" << getWidth() << ">"; | ||
| } | ||
|
|
||
| Type RealType::parse(AsmParser &p) { | ||
| // Default to 64 if no angle bracket group is present. | ||
| unsigned width = 64; | ||
|
|
||
| // If next token is '<', parse the explicit width. | ||
| if (succeeded(p.parseOptionalLess())) { | ||
| if (p.parseInteger(width) || p.parseGreater()) | ||
| return Type(); // signal failure, diagnostic already emitted | ||
| } | ||
|
|
||
| if (width == 32 || width == 64) | ||
| return RealType::get(p.getContext(), width); | ||
| p.emitError(p.getCurrentLocation()) | ||
| << "RealType parser error: expected 64 or 32 bit width, but got " | ||
| << width; | ||
| return Type(); | ||
| } |
There was a problem hiding this comment.
I would make this parameter non-optional. We probably have a few tests that just call out !moore.real at the moment, but these should be fairly easy to fix up to !moore.real<64> 🙂. It's usually a good idea to not have too much syntactic sugar in the IR.
There was a problem hiding this comment.
That would also allow you to continue to use the assemblyFormat and not have custom C++ code for the parser/printer 🙂. Which then also makes the real short form work when the !moore. is clear from context 🙂.
There was a problem hiding this comment.
Or actually coming to think of it, you could just add a f32 and f64 detection to parseMooreType, to make this real type look similar to MLIR's builtin f32 and f64, which we might want to use in the future 😁
There was a problem hiding this comment.
I think I got all of those changed, but feel free to let me know if I missed any!
|
I think I addressed all comments, and CI passes again. Ready to merge unless you got other comments 😄 |
2 participants
This PR disambiguates the
RealTypeinto 32-Bit real types ("shortreal") and 64-Bit real types ("real").It adds support for materializing
moore.shortrealfrom shortreal-specific statements likeas well. This PR also adds support for the builtins
$shortrealtobits,$bitstoshortreal,$realtobitsand$bitstoreal.When working through the builtins mentioned above, I noticed that the
shortrealtype was missing.I decided to add it as
!moore.shortrealsince the distinction between$shortrealtobitsand$realtobitswould not really make sense in the previous type system.To keep backwards compatibility (and also because most real values in Verilog are actually
real, notshortreal), I chose to default the newwidthparameter ofRealTypeto 64 and add a TypeBuilder that accepts "widthless" construction and defaults to 64 Bit.While Section 20.5 "Conversion functions" is unspecific about the domain of the "bits" end of the conversion functions, I infer these two lines to only mean two-valued
bitrather thanlogicvectors:Since this is the first time I add a full-on type to Moore, please let me know if I'm missing anything important!