-
Notifications
You must be signed in to change notification settings - Fork 61
Customize Function
and BasicBlock
to carry both a SPIR-V ID and an index, for O(1) access.
#341
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
Conversation
let (callee_val, result_type, argument_types) = match self.lookup_type(callee.ty) { | ||
// HACK(eddyb) this seems to be needed, but it's not what `get_fn_addr` | ||
// produces, are these coming from inside `rustc_codegen_spirv`? | ||
SpirvType::Function { | ||
return_type, | ||
arguments, | ||
} => { | ||
assert_ty_eq!(self, callee_ty, callee.ty); | ||
(callee.def(self), return_type, arguments) | ||
} | ||
|
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.
This special-case could be removed because it could only be reached by the custom entry-point logic passing a function as the callee, while rustc_codegen_ssa
always expects a function pointer (this matches up with the entry_func
-> self.get_fn_addr(entry_instance)
change in entry.rs
).
While direct calls should IMO be supported as first-class, that would require call
to be able to take Self::Function
as the callee, not Self::Value
, as the only rustc_codegen_ssa
interface to get a Self::Value
for a function is get_fn_addr
(which always creates a pointer).
(This also makes sense if you consider that Self::Value
is really meant to be an IR value, ideally something vaguely-register-like, that can partake in runtime dataflow, and "a function" doesn't fit that without being referred to indirectly, i.e. as a function pointer)
Pull request was converted to draft
…n index, for O(1) access.
057b4c8
to
f8f5301
Compare
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.
Not super familiar with this stuff, but the changes look reasonable. There is only one area that I had an issue following / it is sort of convoluted but it is merely to get to asserts so 🤷
As may be obvious from the first commit, I was hoping to remove the
RefCell
around therspirv::dr::Builder
.(it's still possible, but it would require keeping global and function builders separate, and using different ID ranges, and I almost jerry-rigged some contraption using
spirt::{Context,Type,Const}
but it'd be too much work to port everything over, not to mention new exciting failure modes)Instead, I ended up fixing a long-standing issue with how we expose SPIR-V functions and blocks to
rustc_codegen_ssa
- which is mostly straight-forward, as it already has associated types forFunction
andBasicBlock
, we were just using raw SPIR-V IDs (orSpirvValue
, in the case ofFunction
) for no good reason.After this PR, we're always tracking the
rspirv::dr::Builder
function/block indices, not just IDs (while still using IDs to validate that the index didn't go out of sync due to ad-hoc mutation etc.), so there shouldn't be any reason to iterate through functions (or through a function's blocks) searching for a specific ID.There were already some fast paths before, so this might not have much impact on perf, but I still like the result (if nothing else, it should be harder to misuse, and I even got to remove a silly
call
special-case).