[Migrated] `OpConvertUToPtr` support

[rust-gpu-bot]

Issue automatically imported from old repo: EmbarkStudios/rust-gpu#767
Old labels: t: enhancement,s: qptr may fix
Originally creatd by expenses on 2021-10-13T09:10:25Z

---

I started looking into how OpConvertUToPtr can be implemented - initially for RuntimeArrays. Something like this works well, but is not generic:

unsafe fn load_f32_runtime_array_from_handle(handle: u64) -> &'static mut RuntimeArray<f32> {
    asm!(
        "%f32 = OpTypeFloat 32",
        "%runtime_array = OpTypeRuntimeArray %f32",
        "%runtime_array_ptr = OpTypePointer Generic %runtime_array",
        "%result = OpConvertUToPtr %runtime_array_ptr {handle}",
        "OpReturnValue %result",
        handle = in(reg) handle,
        options(noreturn)
    )
}

I know that @msiglreith implemented loading resources from handles in EmbarkStudios/rust-gpu@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?

[rust-gpu-bot]

Comment from khyperia (CONTRIBUTOR) on 2021-10-13T09:23:02Z

---

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 &'static mut T instead of RuntimeArray, doesn't have to be specialized to RuntimeArray - and probably should return *mut T for saftey, &'static mut T is a little spooky to me~)

[rust-gpu-bot]

Comment from khyperia (CONTRIBUTOR) on 2021-10-13T09:25:05Z

---

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);
    }
}

[rust-gpu-bot]

Comment from expenses (CONTRIBUTOR) on 2021-10-13T10:22:57Z

---

Utgh, I forgot I was on @msiglreith's branch when I tested that 😅

I've been making some progress here: EmbarkStudios/rust-gpu@main...expenses:convert-u-to-ptr

it now fails with alignment issues.

[rust-gpu-bot]

Comment from msiglreith (CONTRIBUTOR) on 2021-10-13T10:23:17Z

---

One issue I had with physical storage addresses was the pointer storage class inference. Afaik the current inference algorithm assumes/requires every pointer being Generic and OpConvertUToPtr requires generating a OpTypePointer out of thin-air in the asm. The default type would be Function which conflicts with the requested pointer type.

[rust-gpu-bot]

Comment from expenses (CONTRIBUTOR) on 2021-10-13T11:15:12Z

---

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.

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 (*y).g but not (*y).a:

error: error:0:0 - Memory accesses with PhysicalStorageBufferEXT must use Aligned.

[rust-gpu-bot]

Comment from expenses (CONTRIBUTOR) on 2021-10-13T12:13:27Z

---

Solved that problem as well - just had to OpCopyMemory instructions. EmbarkStudios/rust-gpu@main...expenses:convert-u-to-ptr now works well for my purposes. It'd be good to see how we can get it to a mergeable state.

[rust-gpu-bot]

Comment from msiglreith (CONTRIBUTOR) on 2021-10-13T12:52:27Z

---

Nice! Memory access instructions in asm blocks accessing physical storage buffers also need handling for the Aligned attribute. Unfortunately these are not covered by the load/memcpy interface.

[rust-gpu-bot]

Comment from khyperia (CONTRIBUTOR) on 2021-10-13T12:58:08Z

---

It'd be good to see how we can get it to a mergeable state.

Unfortunately I think it's pretty far from being mergeable:

1. We probably want to use an integrated-into-the-language version of casting (e.g. as) rather than a function intrinsic. For example, making this work. Still, a function intrinsic is useful for use in user libraries for now, until if/when we get the more integrated version implemented.
2. We need a better way of specifying Aligned on loads than putting them on everything unconditionally.
3. Quite a bit of investigation needs to go into figuring out what non-Logical addressing modes actually imply about what assumptions can be made in the compiler - I'm fairly sure that quite a few places we assume Logical to simplify things. Similarly, assumptions around 32 vs. 64 bit pointers are important - e.g. right now, the branch is using 64 bit pointers with PhysicalStorageBuffer64, except the rust compiler is using 32 bit pointers in the target config.
4. There's a few questionable changes signaling significant underlying problems that need to be investigated and resolved - for example, changing the specializer to default to PhysicalStorageBuffer instead of Function, and changing integer/floats to default to 1-byte alignment.

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 :(

[rust-gpu-bot]

Comment from AidanConnelly (CONTRIBUTOR) on 2021-10-13T17:40:24Z

---

3. Quite a bit of investigation needs to go into figuring out what non-Logical addressing modes actually imply about what assumptions can be made in the compiler - I'm fairly sure that quite a few places we assume Logical to simplify things

To clarify, the memory model is assumed to be logical, or it's assumed to possibly be logical?

[rust-gpu-bot]

Comment from khyperia (CONTRIBUTOR) on 2021-10-14T06:51:07Z 👍: 1

---

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"

[rust-gpu-bot]

Comment from expenses (CONTRIBUTOR) on 2021-10-14T09:20:44Z

---

https://www.khronos.org/registry/SPIR-V/specs/unified1/SPIRV.html#Addressing_Model

PhysicalStorageBuffer64

Indicates that pointers with a storage class of PhysicalStorageBuffer are physical pointer types with an address width of 64 bits, while pointers to all other storage classes are logical.

This seems to imply that we only need to worry about non-logical pointers in the context of PhysicalStorageBuffers, which might make things easier (or not).

[rust-gpu-bot]

Comment from eddyb (CONTRIBUTOR) on 2021-10-20T18:06:37Z

---

One issue I had with physical storage addresses was the pointer storage class inference. Afaik the current inference algorithm assumes/requires every pointer being Generic and OpConvertUToPtr requires generating a OpTypePointer out of thin-air in the asm. The default type would be Function which conflicts with the requested pointer type.

I think the fix for that would be to add an inference rule for OpConvertUToPtr that basically claims that the only storage class it can output is e.g. PhysicalStorageBuffer32 - seems like that would require:

1. adding a new variant to StorageClassPat: https://github.com/EmbarkStudios/rust-gpu/blob/0e0304a2328a1de65e2a6745e6042ea7c03ddb58/crates/rustc_codegen_spirv/src/spirv_type_constraints.rs#L31-L36

• and an import here: https://github.com/EmbarkStudios/rust-gpu/blob/0e0304a2328a1de65e2a6745e6042ea7c03ddb58/crates/rustc_codegen_spirv/src/spirv_type_constraints.rs#L185-L192

2. splitting Op::ConvertUToPtr out of this arm: https://github.com/EmbarkStudios/rust-gpu/blob/0e0304a2328a1de65e2a6745e6042ea7c03ddb58/crates/rustc_codegen_spirv/src/spirv_type_constraints.rs#L427

Op::ConvertUToPtr =>  sig! { (_) -> Pointer(PhysicalStorageBuffer32, _) }

3. handle StorageClassPat::PhysicalStorageBuffer32 here: https://github.com/EmbarkStudios/rust-gpu/blob/0e0304a2328a1de65e2a6745e6042ea7c03ddb58/crates/rustc_codegen_spirv/src/linker/specializer.rs#L1224-L1225

            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 {S} (_) -> Pointer(S, _), where the instruction has a storage class operand, that happens to contain PhysicalStorageBuffer32, and there's also a storage class inference variable in the resulting pointer type, and the use of S in both places links them during inference - but in this case, the instruction itself doesn't have a storage class operand, so we have to fake it)

---

Alternatively, we could hack around this with e.g. OpGenericCastToPtrExplicit to specify the storage class to infer, but then we'd have to remove that instruction since it's Kernel-only.

[rust-gpu-bot]

Comment from eddyb (CONTRIBUTOR) on 2023-03-29T13:30:04Z

---

I've talked about this elsewhere, but my current approach to dealing with pointers is this:

• Introduce qptr ("quasi-pointer") type and associated lower->analyze->lift passes. EmbarkStudios/spirt#24

(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 qptr passes replacing the storage class inference, you could use OpTypePointer PhysicalStorageBuffer64 ... in asm! and it should "just" work (with no need for the types to exactly "match up" elsewhere).