Add lowering rules for {add,sub,or,and} mem, imm on x64
#11043
Conversation
d108c4f to
4a8a20a
Compare
| (rule 1 (x64_add_mem $I64 addr (i64_from_iconst val)) | ||
| (if-let val2 (i32_try_from_i64 val)) |
There was a problem hiding this comment.
This could replace i64_from_iconst with i32_from_iconst to avoid the if-let
There was a problem hiding this comment.
We can't do that because i32_from_const will unconditionally truncate upper bits, but we need to only match this rule when the iconst bits will losslessly fit in an i32.
There was a problem hiding this comment.
I don't think that's correct? The definition of i32_from_iconst uses try_from? (it very much should or we have a lot of incorrect lowering rules)
There was a problem hiding this comment.
i32_from_iconst seems to do the checks but then we have these other cases that do not:
(decl u32_from_iconst (u32) Value)
(extractor (u32_from_iconst x)
(u64_from_iconst (u64_as_u32 x)))
;; ...
(decl i8_from_iconst (i8) Value)
(extractor (i8_from_iconst x)
(i32_from_iconst (i32_as_i8 x)))There was a problem hiding this comment.
So I guess this particular one can change, but I don't think we can blanket apply this reasoning to the other cases
| (rule 2 (x64_add_mem $I32 addr (i32_from_iconst (i8_try_from_i32 val))) | ||
| (x64_addl_mi_sxb_mem addr val)) | ||
| (rule 2 (x64_add_mem $I64 addr (i64_from_iconst val)) | ||
| (if-let val2 (i8_try_from_i64 val)) | ||
| (x64_addq_mi_sxb_mem addr val2)) |
There was a problem hiding this comment.
Would it make sense to add i8_from_iconst to make these two cases one-liners too?
There was a problem hiding this comment.
Similarly, this also needs to be fallible
| fn u64_as_u16_extractor(&mut self, val: u64) -> Option<u16> { | ||
| Some(val as u16) | ||
| } |
There was a problem hiding this comment.
Personally I'm always wary to discard the lower bits with as in lowering rules lest they accidentally get removed. Given that iconst extraction is generally signed could the above rules do (i16_from_iconst imm) and then the immediate to the instruction is (i16_as_u16 imm) to be clear that there's no data loss it's just reinterpreting the sign bit?
There was a problem hiding this comment.
There is no i16_from_iconst and this is only used to implement u16_from_iconst and our existing foo_from_iconst extractors (such as u32_from_iconst) already use lossy as conversions and bring the assumption that the caller is choosing a value based on the correct width for their type context.
I don't think that is really the correct choice, but I also don't want to do something different and make it so we have multiple idioms here. We already have enough trouble from some things being extractors and some being pure partial constructors and not being able to reuse one kind of thing for the other. All of our iconst and numerics stuff could definitely be overhauled but, again, I don't really want to do it in this PR. I'd rather just follow the existing code conventions.
|
Extracting a comment out here since it's not particularly relevant in isolation: I suppose this is fine to land and while on one hand it's not making things worse on the other hand it's also making things worse because it's adding more places to update with a future refactoring. I'll start working on such a refactoring as this is something that's bothered me for quite some time. If you'd like to land this in the meantime I think that's ok too. |
|
I was thinking it would be nice to automatically generate the full combinatorial set of fallible, infallible, checked, and truncating ISLE conversions for numeric types in the meta crate |
|
But yeah, I'll land this now and then we can do the much needed clean up in follow ups. |
No description provided.