-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
refactor: retrieve immediates in new backend #2251
Conversation
Subscribe to Label Actioncc @bnjbvr
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine overall, but see below re: consolidating two matches into one by giving the helper a borrow to the immediates array...
e78d0bb
to
9b8c84b
Compare
This is no change to functionality; the move is necessary in order to return InstructionData immediates in a structure way (see next commit).
This change abstracts away (from the perspective of the new backend) how immediate values are stored in InstructionData. It gathers large immediates from necessary places (e.g. constant pool) and delegates to `InstructionData::imm_value` for the rest. This refactor only touches original users of `LowerCtx::get_immediate` but a future change could do the same for any place the new backend is accessing InstructionData directly to retrieve immediates.
As discussed in bytecodealliance#2251, in order to be very confident that NaN signaling bits are correctly handled by the compiler, this switches `DataValue` to use Cranelift's `Ieee32` and `Ieee64` structures. This makes it a bit more inconvenient to interpreter Cranelift FP operations but this should change to something like `rustc_apfloat` in the future.
9b8c84b
to
c7aac34
Compare
As discussed in bytecodealliance#2251, in order to be very confident that NaN signaling bits are correctly handled by the compiler, this switches `DataValue` to use Cranelift's `Ieee32` and `Ieee64` structures. This makes it a bit more inconvenient to interpreter Cranelift FP operations but this should change to something like `rustc_apfloat` in the future.
c7aac34
to
3c0fe89
Compare
As discussed in #2251, in order to be very confident that NaN signaling bits are correctly handled by the compiler, this switches `DataValue` to use Cranelift's `Ieee32` and `Ieee64` structures. This makes it a bit more inconvenient to interpreter Cranelift FP operations but this should change to something like `rustc_apfloat` in the future.
@cfallin, here is what I came up with after we discussed this last week. This change:
DataValue
into cranelift-codegen so that I can use it as a value wrapperInstructionData
the ability to find immediate values in the format structuresLowerCtx::get_immediate
to grab the value from the instruction (and the DataFlowGraph for vector immediates)I felt like this approach provides the new backend a more abstract view of immediates instead of munging directly in the
InstructionData
but I didn't perform the refactor on the rest of the places the new backend grabs immediates because I wanted to get some feedback first.