search

0xPolygonMiden / miden-vm

STARK-based virtual machine
MIT License
612 stars 150 forks source link

u32 bitwise operations should accept immediate values #1301

Closed hackaugusto closed 1 week ago

hackaugusto commented 3 months ago

For example, u32and may be used with a mask, and it is nicer to specify u32and.0b1100 or u32and.ACCOUNT_TYPE_MASK.

Fumuran commented 3 weeks ago

@hackaugusto what types should be allowed to be represented as binary values? I think for u8, u16, u32 and Felt it is fine, but will there be a use case when we want to represent a Word as a binary value?

hackaugusto commented 3 weeks ago

I think for the MASM it makes sense to support just Felt. Meaning the text is parsed, decoded to a Felt, if the value would overflow than we get a syntax error, otherwise we convert it to a Felt like it would be normally

bobbinth commented 3 weeks ago

Since these are u32 operations - shouldn't we always treat immediate values as u32 values? For example, in u32and.ACCOUNT_TYPE_MASK, I'm not sure anything bigger than a u32 would make sense for ACCOUNT_TYPE_MASK.

If it simplifies things, we can store immediates in the AST as Felt's but I think if the user tries to use anything greater than u32 value in these operations, it should be a compile-time error.

Fumuran commented 3 weeks ago

I agree, it makes sense to always treat immediates here as u32, also it is even more simple to store u32 that Felt, so this shouldn't be a problem.

Also, one clarification: is it fine to support only decimal or binary values as immediates for u32 bitwise ops, without hex values?

bobbinth commented 3 weeks ago

I think we should try to support hex values (we already support them for constants).

bitwalker commented 3 weeks ago

The current parser already makes it trivial to add support for more representations than decimal and hex immediates (which are already supported). It would be nice to support binary for some of the u32 ops as well, particularly the bitwise ops, but that will need to be added to the lexer (the 0b prefix is easy to recognize, so it should be trivial to support).

I would extend all immediate integers to allow this (just like today such immediates support both decimal and hex forms), and simply catch if the resulting integer overflows whatever integral type it is supposed to represent. That will keep the existing parser clean, and also make the syntax more uniform (i.e. if you can use a given literal syntax with one op, you should be able to use it with all ops that accept an immediate of that type).

Otherwise +1 to u32 immediates here, rather than Felt

Fumuran commented 1 week ago

Closed by #1362.