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

Allow constant references (to constant data), when they're valid SPIR-V. #586

Merged
merged 5 commits into from
Apr 12, 2021

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Apr 9, 2021

This is trickier than it should reasonably be, because of some SPIR-V limitations.

A constant like &&123 is valid in SPIR-V and generates something like this (types omitted for brevity):

        %int_123 = OpConstant _ 123
    %ref_int_123 = OpVariable _ Private %int_123
%ref_ref_int_123 = OpVariable _ Private %ref_int_123

But a constant like &(&1, &2) isn't - its SPIR-V would have to be this, which doesn't pass validation:

    %int_1 = OpConstant _ 1
%ref_int_1 = OpVariable _ Private %int_1
    %int_2 = OpConstant _ 2
%ref_int_2 = OpVariable _ Private %int_2
     %pair = OpConstantComposite _ %ref_int_1 %ref_int_2
 %ref_pair = OpVariable _ Private %ref_pair

The problem isn't even %ref_pair, but %pair itself: OpConstantComposite must have OpConstant... fields, but not (global, i.e. module-scoped) OpVariable fields (even if the storage class is Private).

This is pretty bizarre IMO, and I'm not 100% convinced it's intentional - it's possible the first case (with the nested OpVariables) was never meant to be legal either.


Regardless of whether the rules make sense or not, this PR accounts for them, and you can see in the UI tests some simple examples what is allowed and what isn't (the == examples were the original motivation for this PR).

I've even tested that adding multiple extraneous references to the color constants in mouse-shader still works, but I suspect the loads get constant-folded anyway, so it's not much of a test.

@eddyb eddyb requested a review from khyperia April 9, 2021 00:03
@eddyb eddyb enabled auto-merge (rebase) April 9, 2021 06:35
Copy link
Contributor

@khyperia khyperia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what the fuck

[OpVariable instruction spec]: Initializer is optional. If Initializer is present, it will be the initial value of the variable’s memory content. Initializer must be an from a constant instruction or a global (module scope) OpVariable instruction.

@@ -214,15 +214,18 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let elem_pat = self.memset_const_pattern(&self.lookup_type(element), fill_byte);
self.constant_composite(
ty.clone().def(self.span(), self),
vec![elem_pat; count as usize],
iter::repeat(elem_pat).take(count as usize),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ ty

// FIXME(eddyb) materialize this at runtime, using
// `OpCompositeConstruct` (transitively, i.e. after
// putting every field through `SpirvValue::def`),
// if we have a `Builder` to do that in.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah gosh, this would be nice, been dreaming about this possibility for ages, haha - seems like with this system, we're actually kind of close to being able to do so

Ok(())
}
SpirvConst::Undef => {
// FIXME(eddyb) check that the type supports `OpUndef`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, are there any types that cannot be made by OpUndef? Intuitively, there absolutely should be, but I don't think the spec actually mentions anything.

// Bidirectional maps between `SpirvConst` and the ID of the defined global
// (e.g. `OpConstant...`) instruction.
// NOTE(eddyb) both maps have `WithConstLegality` around their keys, which
// allows getting that legality information without additional lookups.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: around their values?

@eddyb eddyb merged commit 7c568ab into EmbarkStudios:main Apr 12, 2021
@eddyb eddyb deleted the const-data branch April 12, 2021 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants