feat: lower i64.extend*_s through wasm dialect#1008
Conversation
dialects/wasm/src/ops.rs
Outdated
| implements(UnaryOp, InferTypeOpInterface, MemoryEffectOpInterface, Foldable, OpPrinter) | ||
| )] | ||
| pub struct I32ExtendS { | ||
| pub struct ExtendS { |
There was a problem hiding this comment.
Let's rename this SignExtend since the correlation to the actual Wasm opcode is basically gone, and that name is clearer than ExtendS/extend_s IMO.
In general, I would prefer to have more precise ops, and factor out the common parts with functions/traits/whatever, rather than create this kind of "mega" ops that handle all sorts of variations, since it complicates handling of those ops in many ways, and sometimes precludes implementing traits for them (not really in this case though). We do have legacy ops like this (namely the various generalized cast operations), but we can likely remove some of those once the Wasm dialect is complete (at least hir.cast, since that existed primarily to handle cases that will now be handled with more precise ops).
Anyway, it's not necessary to change this op, since it is still reasonably narrowly scoped - but just something to bear in mind going forward. Typically it is easier to write canonicalizations and optimizations on more precise ops, rather than on general ops, and more efficient to match as well when those rules can be trivially excluded/included based on the opcode, rather than needing to evaluate properties of the operation in detail. Basically, it isn't essential to factor out duplication in opcodes, just in the implementation of their behavior (i.e. if we had distinct opcodes for every pair of source and target type like Wasm does, we could still factor out the implementation details of those ops using generic functions).
There was a problem hiding this comment.
Renamed it to SignExtend. I see your point and will bear it in mind!
I wouldn't mind splitting it up by restoring I32ExtendS and adding I64ExtendS. Either as a follow-up or once SignExtend being too general becomes an issue.
bitwalker
left a comment
There was a problem hiding this comment.
Looks good, just one change (renaming the op to SignExtend), and we can merge this
Generalize
I32ExtendStoExtendSto support bothi32.extend*_sandi64.extend*_ssign-extension instructions:I32ExtendS→ExtendSdst_tyattribute to explicitly track the destination typei64.extend(8|16|32)_sin the Wasm frontendExtendSnow handle variable destination typesChanges in generated code
None as the
i64.extend(8|16|32)_swere not supported previously.