-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cranelift: simplify opcode set by removing _imm
variants
#3250
Comments
One of the arguments that @bjorn3 mentioned is that, the However, I also think that if we do split the op with the builder, the readability impact is somewhat minimized since the E.g:
Another thing to consider is that we use a See: wasmtime/cranelift/filetests/filetests/runtests/heap.clif Lines 156 to 159 in 0771abf
|
This likely won't be true after optimizations like GVN. |
I like this proposal. I generally like the idea of having clif be very riscy and full of micro-ops and then letting isel lowering choose the appropriate macro-op for the target arch. This change seems inline with that.
Good catch. If we removed
Or we could go the other direction and allow every operand to every instruction to be either an ssa value or an inline constant... |
Very out-of-the-box and I like it! I think I've seen a JIT engine that worked like this (I forget where?). It would be a pretty deep change throughout the compiler -- everything operates on |
One option would be to keep using |
Two random thoughts: One of the original motivations for the If
and one can immediately tell the multiply is single-use, without scanning the rest of the function. |
In various discussions, we have come to the conclusion that "combo ops" generally cost more than they are worth. When one CLIF opcode simply expresses the combination of two other opcodes, it (i) expands the set of opcodes that all consumers of CLIF must handle, but (ii) adds minimal value, because one can pattern-match if one needs to handle the combination specially.
So far we have not really discussed the
op_imm
opcodes (e.g.,iadd_imm
andisub_imm
) in this context. They are currently converted in the "simple legalization" pass used by new backends intoiconst
+ op, so the backends do not need to actually handle them; but this separate pass is awkward and shouldn't be necessary.Instead, it might be better to remove the combo opcodes, but provide backward compatibility (and convenience) to producers by adding combination methods to the instruction builder that generate the two opcodes. So
InstBuilder::iadd_imm
would generate aniconst
and aniadd
, for example.This would require some work in the meta crate but is probably feasible. The main downside is that the CLIF becomes slightly more inflated earlier in the pipeline, but we expand it before lowering anyway, so it may actually be better to generate it in the final form and avoid the edit.
cc @abrown @afonso360 @bjorn3 from earlier discussion
The text was updated successfully, but these errors were encountered: