cfallin opened issue #3250:
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
afonso360 commented on issue #3250:
One of the arguments that @bjorn3 mentioned is that, the
_imm
forms are much more readable in IR textual form. I agree with him.However, I also think that if we do split the op with the builder, the readability impact is somewhat minimized since the
const
is directly above theop
, and the reader doesn't have to look to far for the value.E.g:
v123 = iconst.i32 0xFFFF_0000 v124 = iadd.i32 v0, v123
Another thing to consider is that we use a
iadd_imm
like symbol to denote offsets in global values. This may become confusing ifiadd_imm
is no longer an instruction.
afonso360 edited a comment on issue #3250:
One of the arguments that @bjorn3 mentioned is that, the
_imm
forms are much more readable in IR textual form. I agree with him.However, I also think that if we do split the op with the builder, the readability impact is somewhat minimized since the
const
is directly above theop
, and the reader doesn't have to look too far for the value.E.g:
v123 = iconst.i32 0xFFFF_0000 v124 = iadd.i32 v0, v123
Another thing to consider is that we use a
iadd_imm
like symbol to denote offsets in global values. This may become confusing ifiadd_imm
is no longer an instruction.
bjorn3 commented on issue #3250:
However, I also think that if we do split the op with the builder, the readability impact is somewhat minimized since the const is directly above the op, and the reader doesn't have to look too far for the value.
This likely won't be true after optimizations like GVN.
fitzgen commented on issue #3250:
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.
Another thing to consider is that we use a
iadd_imm
like symbol to denote offsets in global values. This may become confusing ifiadd_imm
is no longer an instruction.Good catch. If we removed
_imm
-suffixed instructions, it would probably make sense to rename this fromiadd_imm
tooffset_of_global
or something.However, I also think that if we do split the op with the builder, the readability impact is somewhat minimized since the
const
is directly above theop
, and the reader doesn't have to look too far for the value.Or we could go the other direction and allow every operand to every instruction to be either an ssa value or an inline constant...
cfallin commented on issue #3250:
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
Value
s now -- but something we could consider if we ever have a wider design-reconsideration phase for CLIF.
sunfishcode commented on issue #3250:
One option would be to keep using
Value
, and add anImmediate
arm toValueDef
.
sunfishcode commented on issue #3250:
Two random thoughts:
One of the original motivations for the
_imm
instructions was compile time -- doing a very simple pattern-match early on in the compiler reduces the number of instructions that anything else in the compiler that visits every instruction has to visit.If
_imm
instructions are removed, one option to recover the readability would be to introduce infix notation in the clif syntax for single-use arithmetic and constant instructions.v4 = v3 * v2 + 5
is much easier to read at a glance thanv6 = imul v3, v2 v4 = iadd_imm, v6, 5
and one can immediately tell the multiply is single-use, without scanning the rest of the function.
akirilov-arm labeled issue #3250:
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
Last updated: Dec 23 2024 at 12:05 UTC