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

Constant propagation #97

Closed
kvark opened this issue Jul 3, 2020 · 4 comments · Fixed by #2309
Closed

Constant propagation #97

kvark opened this issue Jul 3, 2020 · 4 comments · Fixed by #2309
Assignees
Labels
area: middle Intermediate representation kind: feature New feature or request

Comments

@kvark
Copy link
Member

kvark commented Jul 3, 2020

We should allow constant expressions for array sizes. This needs IR changes:

  • enum ConstExpression that takes a subset of Expression that's valid for constant propagation
  • the Expression::Const(Handle<ConstExpression>) to transition from regular expressions (instead of the current Expression::Constant)
  • separate pub const_expressions: Arena<ConstExpression> in a function
@kvark kvark added kind: feature New feature or request area: middle Intermediate representation labels Jul 3, 2020
@kristoff3r
Copy link
Contributor

I have been thinking about this during the weekend, and I don't think your outline is the best approach. As far as I can tell, you get problems no matter which subset of Expression you choose for ConstExpression:

  • If you only allow things that are constant by construction (e.g. Constants, BinaryOp and UnaryOp), then the frontend needs to do inlining, and at that point it might be easier to just evaluate the expression.
  • If you extend it to include e.g. LocalVariable and GlobalVariable, then you have no guarantee that it is actually a constant fragment, and then the type doesn't really make sense.

In both cases we get friction from having duplication between Expression and ConstExpression, and we also need to do extra work if we want constant folding optimizations for normal expressions.

Instead I propose that we change ArraySize::Static to contain a normal Expression. Then we can implement constant folding for all expressions in the IR, and have a step in the validator that checks that all expressions in ArraySize::Static are Constant after the constant folding.

Does that make sense, or have I missed something?

@kvark
Copy link
Member Author

kvark commented Jul 6, 2020

I thought this is something we'll need, but now there is a bit of doubt, see gpuweb/gpuweb#905
It doesn't necessarily block us (if we have constant propagation but WGSL doesn't, we can work with that, and the other way around - too) though.

If you only allow things that are constant by construction (e.g. Constants, BinaryOp and UnaryOp), then the frontend needs to do inlining, and at that point it might be easier to just evaluate the expression.

I'm not sure what you mean here by "the frontend needs to do inlining", please elaborate!

The way I see it: when the front-end encounters a constant, its expression parsing gets the "constant" mode. So seeing A + B constants, it will use the constant expression BinaryOp. If any of A or B are not constants, it will switch to the regular expression mode and convert any previously constant A or B into expressions by doing Expression::Constant variant. This seems doable without too much code or complex logic. Please tell me if you think otherwise:)

If you extend it to include e.g. LocalVariable and GlobalVariable, then you have no guarantee that it is actually a constant fragment, and then the type doesn't really make sense.

Variables are not constants by definition :) constant expression enum doesn't need to include those variants.

In both cases we get friction from having duplication between Expression and ConstExpression

I don't know how much duplication there's really going to be. Roughly, we'll need the following variants:

  • AccessIndex
  • Constant
  • Compose
  • Unary
  • Binary
  • Intrinsic

This doesn't look too scary to me, although I agree that some friction will be present.
The dot/cross product need to be removed from the Expression variants anyway, to be in line with other stdlib functions and intrinsics.

and we also need to do extra work if we want constant folding optimizations for normal expressions.

Wouldn't this make it easier? Constant folding would require precisely the same separation of constant mode vs regular expression mode that I described, so it seems like it would be a piece of cake to get if we need it.


Concluding a bit, I know what can be done here, but I don't know (or have a strong feeling) on what should be done.

We can proceed with your GLSL changes and see what WebGPU group decides. It could be that we'll just leave ArraySize::Static to store a plain integer value.

@kvark
Copy link
Member Author

kvark commented Nov 3, 2020

So, the group's resolution for now is to not have the constant propagation in WGSL.
However, the group also decided to have the specialization constants... and now we are sorta in trouble.
Say, WGSL or SPIR-V defines a private variable at global scope, initialized by a constant that happens to be a specialization constant. We don't have Arena<Expression> outside of functions, so we can't have Handle<Expression> in global scope. At the same time, we can't have a pure value in there, since it depends on something we don't know at compile time (the value of the spec constant). So it sounds to me that we'll need a ConstExpression enum anyway!

@kvark kvark self-assigned this Nov 3, 2020
@kvark
Copy link
Member Author

kvark commented Nov 3, 2020

Another option would be to remove the serializer entirely from the local variables... which is fine, semantically speaking, since everything is expected to be zero-initialized, at least in wgpu. It may be less fine from the performance perspective, since assigning a value would map to OpStore, while the initializer is free in SPIR-V.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: middle Intermediate representation kind: feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants