Skip to content
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

Inliner heuristic for "illegal pointer argument" is incomplete. #1021

Closed
eddyb opened this issue Apr 3, 2023 · 1 comment · Fixed by #1023
Closed

Inliner heuristic for "illegal pointer argument" is incomplete. #1021

eddyb opened this issue Apr 3, 2023 · 1 comment · Fixed by #1023
Assignees
Labels
t: bug Something isn't working

Comments

@eddyb
Copy link
Contributor

eddyb commented Apr 3, 2023

This is technically not a new issue, and most of it was fixed in:

But if we take the test for that (tests/ui/lang/core/ref/member_ref_arg.rs) and add another function g:

struct S {
    x: u32,
    y: u32,
}

fn f(x: &u32) {}

fn g(xy: (&u32, &u32)) {}

#[spirv(fragment)]
pub fn main() {
    let s = S { x: 2, y: 2 };
    f(&s.x);
    g((&s.x, &s.y));
}

Then only the (existing) f call is inlined, but the g call still produces:

error: error:0:0 - Pointer operand '22[%22]' must be a memory object declaration

This is because #777 banned specifically OpAccessChain function arguments (i.e. &s.x), but in the case of g, the pair is created and then (because it has ScalarPair ABI) the two pointer fields are extracted to call g.

Based on my reading of the SPIR-V standard, the only way to be fully correct here is to only allow OpVariables and OpFunctionParameters (instead of banning one thing or another).

cc @Shfty (who ran into this while doing some type/trait metaprogramming which I guess involved ref-pairs)

@eddyb eddyb added the t: bug Something isn't working label Apr 3, 2023
@eddyb eddyb self-assigned this Apr 3, 2023
@eddyb
Copy link
Contributor Author

eddyb commented Apr 3, 2023

The relevant validation rules are:

Any pointer operand to an OpFunctionCall must be

(note that the existing check introduced by #777 already rules out the second case, because that would involve OpAccessChain, so we can treat it like "memory object declaration" is the only allowed case)


The Shader validation rules are even stricter than that though:

All declared types are restricted to those types that are, or are contained within, valid types for an OpVariable Result Type or an OpTypeFunction Return Type.

So the inliner checks can probably partition types into "pointer to OpVariable's contents" (valid for function parameters, too, for a subset of storage classes) and "pointer-free type" (valid for function return types).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: bug Something isn't working
Projects
None yet
1 participant