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

Inline globals #857

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Inline globals #857

wants to merge 10 commits into from

Conversation

molikto
Copy link
Contributor

@molikto molikto commented Apr 5, 2022

  • warn on inline(never) that are not respected
  • inline global variables so that we can inline less functions caused by illegal arg/return type

fix #847

tests

My shader which compile to a single file of 2500 lines (dis glsl) now respect the inline(never) and result in 1400 lines of glsl

TODO:

  1. some compiletests produce different result??

  2. I was hoping by force inlining less functions, some shader can compile faster, but it is not the case yet, (need to look into if it actually result in less inlines?) af3cf2b74e8ef4f4bd98a3cd2cda64b9379d20d7 compile time: before: 222s -> after: 287s

@molikto molikto requested a review from eddyb as a code owner April 5, 2022 04:59
@eddyb
Copy link
Contributor

eddyb commented Apr 5, 2022

some compiletests produce different result??

For the SPIR-V disasm checks, that makes sense - you're removing some inlining requirements.


I've tried looking at this PR, and while I believe the general idea makes sense (i.e. as a form of "parameter weakening", all call sites agreeing on a quasi-constant value for some parameter does allow removing the parameter from the signature), I can't easily reason about the specifics of the code in this PR at this time.


At the very least, I feel like it should go into the param_weakening module:

//! Interprocedural optimizations that "weaken" function parameters, i.e. they
//! replace parameter types with "simpler" ones, or outright remove parameters,
//! based on how those parameters are used in the function and/or what arguments
//! get passed from callers.

And use the CallGraph abstraction to correctly visit the module in the right order for the right operations. Ideally we would have a framework for computing general function parameter weakenings from either callee uses (like the existing remove_unused_params) or call sites, and correctly propagating them through any number of nested calls.


I'm leaving this comment now as-is because I'm worried I might not be able to meaningfully review this any time soon, or at least not without doing some of the generalization work, to be able to convince myself this transform won't have subtle failure modes.

@eddyb eddyb added the s: waiting on review PRs that blocked on a team member's review. label May 21, 2022
@eddyb eddyb added the s: qptr may fix This might be fixed by the "qptr" experiment (https://github.com/EmbarkStudios/spirt/pull/24) label Mar 29, 2023
@AdrianEddy
Copy link

Any update on this? I have a shader which compiles to 6000 lines of GLSL or WGSL code, while hand-crafted one was only 300 lines, and it would really help to not inline the functions, as my only parameter to the inlined functions is the single uniform struct reference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: qptr may fix This might be fixed by the "qptr" experiment (https://github.com/EmbarkStudios/spirt/pull/24) s: waiting on review PRs that blocked on a team member's review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#[inline(never)] ignored when using borrowed push constant as argument.
3 participants