-
Notifications
You must be signed in to change notification settings - Fork 246
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
OpConvertUToPtr
support
#767
Comments
I believe this will work: unsafe fn load_runtime_array_from_handle<T>(handle: u64) -> &'static mut RuntimeArray<T> {
let result: *mut RuntimeArray<T>;
asm!(
"{result} = OpConvertUToPtr typeof{result} {handle}",
handle = in(reg) handle,
result = out(reg) result,
);
&mut *result
} However, this runs into the issue that ConvertUToPtr requires non-Logical addressing, which will require (significant?) compiler changes to support. Also, I know at least the Addresses capability is not supported on Vulkan, and I'm not sure about PhysicalStorageBufferAddresses, would have to check (one of those two things needs to be supported for ConvertUToPtr to work). But, this is at least the way to write such a thing. (I think you could probably just straight up return |
Here's a compiletest, in case we need it in the future (currently fails with "OpConvertUToPtr not supported in Logical addressing") // build-pass
// compile-flags: -C target-feature=+PhysicalStorageBufferAddresses,+ext:SPV_KHR_physical_storage_buffer
use spirv_std::*;
unsafe fn convert_u_to_ptr<T>(handle: u64) -> *mut T {
let result: *mut T;
asm!(
"{result} = OpConvertUToPtr typeof{result} {handle}",
handle = in(reg) handle,
result = out(reg) result,
);
result
}
#[spirv(fragment)]
pub fn main(out: &mut u32) {
unsafe {
let x: *mut RuntimeArray<u32> = convert_u_to_ptr(100);
*out = *(*x).index(0);
}
} |
Utgh, I forgot I was on @msiglreith's branch when I tested that 😅 I've been making some progress here: main...expenses:convert-u-to-ptr it now fails with alignment issues. |
One issue I had with physical storage addresses was the pointer storage class inference. Afaik the current inference algorithm assumes/requires every pointer being |
Okay, most issues have been resolved, but I'm still getting alignment errors with some struct fields: #[repr(C)]
pub struct Struct {
pub a: Mat4,
pub b: Mat4,
pub c: Vec3,
pub d: f32,
pub e: u32,
pub f: u32,
pub g: u32,
pub h: bool,
}
#[spirv(fragment)]
pub fn main(out: &mut u32) {
unsafe {
let x: *mut RuntimeArray<u32> = arch::convert_u_to_ptr(100);
let y: *mut Struct = arch::convert_u_to_ptr(200);
let mat = (*y).a;
*out = *(*x).index(0) + (*y).g;
}
} It's happy with error: error:0:0 - Memory accesses with PhysicalStorageBufferEXT must use Aligned. |
Solved that problem as well - just had to |
Nice! Memory access instructions in asm blocks accessing physical storage buffers also need handling for the |
Unfortunately I think it's pretty far from being mergeable:
Sadly, I don't think this is very high on Embark's priority list, so I'm not sure how much help on the above I'll be able to provide :( |
To clarify, the memory model is assumed to be logical, or it's assumed to possibly be logical? |
Assumed to be logical - as in, "ah, there's a really complicated to deal with situation here that's difficult to implement, but, this situation can never happen with logical, only physical, so we're not going to deal with it right now" |
https://www.khronos.org/registry/SPIR-V/specs/unified1/SPIRV.html#Addressing_Model
This seems to imply that we only need to worry about non-logical pointers in the context of |
OpConvertUToPtr
support for RuntimeArray
sOpConvertUToPtr
support ~for RuntimeArray
s~
OpConvertUToPtr
support ~for RuntimeArray
s~OpConvertUToPtr
support ~~for RuntimeArray
s~~
OpConvertUToPtr
support ~~for RuntimeArray
s~~OpConvertUToPtr
support
I think the fix for that would be to add an inference rule for
Op::ConvertUToPtr => sig! { (_) -> Pointer(PhysicalStorageBuffer32, _) }
StorageClassPat::PhysicalStorageBuffer32 => {
let mut m = Match::default();
// HACK(eddyb) choice of `0` makes this potentially overlap
// with `Var(0)` and a different mechanism should be added.
let found = m.storage_class_var_found
.get_mut_or_default(0);
found.push(storage_class);
found.push(InferOperand::Concrete(CopyOperand::StorageClass(StorageClass::PhysicalStorageBuffer32)));
m
} (this effectively emulates something like Alternatively, we could hack around this with e.g. |
I've talked about this elsewhere, but my current approach to dealing with pointers is this: (there's a lot there you could read, but the short version is that the "storage class inference" in Rust-GPU is a dead-end, and erasing pointer types can be far more effective and flexible) With the |
I started looking into how
OpConvertUToPtr
can be implemented - initially forRuntimeArray
s. Something like this works well, but is not generic:I know that @msiglreith implemented loading resources from handles in main...msiglreith:glace, but it looks like that requires a lot of codegen changes - ideally we should keep things at the
asm!
layer if possible.Is there a way to make the above function generic over
T
, or will this require a few codegen changes?The text was updated successfully, but these errors were encountered: