Allow binding immediates to instructions#1012
Allow binding immediates to instructions#1012abrown merged 7 commits intobytecodealliance:masterfrom
Conversation
| fn build_fake_instruction( | ||
| inputs: Vec<OperandKindFields>, | ||
| outputs: Vec<OperandKindFields>, | ||
| ) -> Instruction { |
There was a problem hiding this comment.
Not very happy with this type of thing but I wanted to have some way of verifying the behavior; perhaps there is a less cumbersome way to build up a fake instruction?
There was a problem hiding this comment.
Agreed it's a bit cumbersome to do this at the moment, because there's no helpers for this. Maybe this function could be shared with some other test submodules, so we could start testing this better. It could also be part of tests that live under cranelift-codegen/meta/src/tests.
There was a problem hiding this comment.
It could also be part of tests that live under cranelift-codegen/meta/src/tests.
I don't see that right now... you mean create it?
bnjbvr
left a comment
There was a problem hiding this comment.
Thanks! (Sorry for the long review time.)
Approving, in the spirit of moving things forward; we can work on incremental improvements once this has landed.
It'd be easy to mistake an immediate for another one in the current state, so I wonder if the immediate's name should be in the bind() method call too. For what it's worth, even binding values today can show complicated for the exact same reason; it's even worse for values since the first bound type is the controlling type (~= result type), which can be different from the first input's type.
Also, it's possible to intertwine binding immediates and values in any order, which may be a bit odd. It's not something new either: when creating a format, it's possible to alternate between values and immediates in the same way, with the same effect. (Getting rid of this double, distinct ordering may simplify the code generation's code, but that's something to think about for the future.)
for the change in this PR to work on uimm128 we either need special code for extracting the uimm128 from the constant pool or we need to avoid putting the values in the constant pool until later--thoughts?
We might need to add special code to extract the value from the constant pool indeed; deferring the write to the constant pool would break the Instruction size invariant, since it'd need to store the constant inline, right?
to bind instructions to IntCC/FloatCC codes we might want to consider pulling out condcodes.rs into a new module shared by both meta and cranelift-codegen--thoughts on that?
Now it's been long enough that this has been done for other reasons :).
| /// Verify that binding types to the instruction does not violate the polymorphic rules. | ||
| fn verify_polymorphic_binding(&self) -> Result<(), String> { | ||
| /// Verify that the bindings for a BoundInstruction are correct. | ||
| fn verify_bindings(&self) -> Result<(), String> { |
There was a problem hiding this comment.
What do you think of adding a boolean parameter (or enum with 2 variants) telling whether the check should concern values or immediates? There's no need to check both at the time for every bind() call, since we can only bind to one of them at a time.
There was a problem hiding this comment.
How about I do this when I work on https://github.com/CraneStation/cranelift/issues/1130?
| fn build_fake_instruction( | ||
| inputs: Vec<OperandKindFields>, | ||
| outputs: Vec<OperandKindFields>, | ||
| ) -> Instruction { |
There was a problem hiding this comment.
Agreed it's a bit cumbersome to do this at the moment, because there's no helpers for this. Maybe this function could be shared with some other test submodules, so we could start testing this better. It could also be part of tests that live under cranelift-codegen/meta/src/tests.
| } | ||
|
|
||
| #[test] | ||
| fn ensure_bound_instructions_can_bind_immediates() { |
There was a problem hiding this comment.
Nice! Can you add tests that try:
- to bind more value types than the instruction's number of values?
- ditto for immediates
- to bind an immediate of the wrong type?
There was a problem hiding this comment.
to bind an immediate of the wrong type
Right now we have no idea of the immediate type in OperandKindFields::ImmValue so we can't check. And that worries me: the same is true for the value types. I think the next iteration should add that logic to verify_bindings.
I did add tests for too many types and immediates.
f108598 to
5c1f2d7
Compare
The vector usages are left with a helper method, Bindable::bind_vector(), because a second parameter is passed (i.e. bector bit width)
5c1f2d7 to
a2bf710
Compare
|
Thanks for answering all the comments! Happy to land it the way it is, and we can improve it over time. |
After discussing over in abrown#3 (@bnjbvr, I believe I have dealt with the feedback you left there in these re-worked changes), I think this is ready for some more eyes. The purpose of the change is to make it easier to encode instructions for specific immediate values. Currently, one has to build an
InstructionPredicatefrom a constructor function (e.g.InstructionPredicate::new_is_all_zeroes_128bit) and attach this to theEncodingBuilderas an encoding predicate; with this change, one could writee.enc(inst.bind(Immediate::UInt8(0)), ..., e.g.This change:
Bindabletrait and associated conversions to unify all of thebind_any,bind_ref, etc. to a singlebind(though I have retained a helperbind_vectordue to it having a second parameter)bindtheEncodingBuilderadds the appropriate predicate to ensure that the encoding is only applied if the immediate value in the code matches the bound immediate valueencodings.rsfiles to use the newbindmethodI was hoping to get feedback before I go any further, but I see a few potential problems ahead:
uimm128I replaced the actual value with aConstanthandle very early on during parsing; for the change in this PR to work onuimm128we either need special code for extracting theuimm128from the constant pool or we need to avoid putting the values in the constant pool until later--thoughts?IntCC/FloatCCcodes we might want to consider pulling outcondcodes.rsinto a new module shared by bothmetaandcranelift-codegen--thoughts on that?