cranelift: Add support for parsing i128 data values #3351
Conversation
github-actions
bot
added
the
cranelift
afonso360
force-pushed
the
parser-i128
branch
from
6c75e6c to
8c488a8
Compare
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Thanks for this, especially all the really tedious test updates! Just a few comments below.
| } | ||
|
|
||
| // Match and consume an i128 immediate. | ||
| fn match_imm128(&mut self, err_msg: &str) -> ParseResult<i128> { |
There was a problem hiding this comment.
I see the duplication is pre-existing, but it's also unfortunate to continue it here. While it may expand the scope of this PR a bit, I wonder if there's a way to write a single polymorphic implementation? Maybe it can take the signed/unsigned types and require FromStr / FromHex / whatever encapsulates wrapping_neg?
There was a problem hiding this comment.
I tried doing this as a generic function, but it was proving to be somewhat difficult, most of the functions that we need / could use are not in separate traits, but as functions associated with the primitive types. FromStr works, but FromHex doesen't exist. Casting between signed and unsigned types in a generic function, as far as I can tell is a no go.
We could solve pretty much all of these problems by adding a dependency on the num crate, but I don't think this justifies it.
I've implemented this as a macro, hope that is ok.
afonso360
force-pushed
the
parser-i128
branch
from
8c488a8 to
7c14910
Compare
Transforming this into a generic function is proving to be a challenge since most of the necessary methods are not in a trait. We also need to cast between the signed and unsigned types, which is difficult to do in a generic function. This can be solved for example by adding the num crate as a dependency. But adding a dependency just to solve this issue seems a bit much.
afonso360
force-pushed
the
parser-i128
branch
from
7c14910 to
3a4ebd7
Compare
| } | ||
|
|
||
| // Match and consume an i128 immediate. | ||
| fn match_imm128(&mut self, err_msg: &str) -> ParseResult<i128> { |
2 participants
Hey,
This PR adds support for parsing
i128data values, it also cleans up thei128test suite to pass those values directly to the functions. (We previously passed the bottom and top half ini64's andiconcatthem together)This causes a bunch of noise in the diff (sorry!). I've separated this into two commits, the first one with the parser changes, and the second one with test suite changes. It might be easier to review it that way.
Since we now pass i128's directly we need to enable the
enable_llvm_abi_extensionsflag. This is because thewindows_fastcallABI does not specify how to passi128values to functions. This is only necessary for the tests to pass on x86_64 windows.It doesn't add support for
iconst.i128since its instruction format (UnaryImm) doesen't allow us to include a 128 bit value. We would need to change it toUnaryConstfor that.