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

dead code elimination #68

Open
mighdoll opened this issue Jan 22, 2025 · 2 comments
Open

dead code elimination #68

mighdoll opened this issue Jan 22, 2025 · 2 comments

Comments

@mighdoll
Copy link
Contributor

(I don't think this is urgent for M1, but wanted to save the issue for later discussion.)

A couple of arguments for eventually spec'ing that dead code should be removed:

  1. might pull in unrelated const_assert statements (and cause unexpected failures when the linked result is passed to naga/tint)
  2. might lead to untested conditions (and thus fail at naga/tint)

Here's an example of the second untested conditions case. foo imports something unrelated from util, but that leads to a set of untested conditions which would fail to compile in naga/tint:

/* -- main.wesl -- */
import super::foo::bar;

@compute @workgroup_size(1)
main() { bar(); }


/* -- foo.wesl -- */
import super::util;

fn bar() { util::zap(); } 


/* -- util.wesl -- */
import super::digger::dig;

fn zap() { }

// proposed: this is not linked 
//   Let's say only cond1 is true.
//   This would fail to compile in tint/naga compile if it were linked
@if (cond1)
fn dig_hole() { dig(); }


/* -- digger.wesl -- */
@if (cond2) import super::util::shovel1::shovel as shove;
@if (cond3) import super::util::shovel2::shovel as shove;

@if (cond1)
fn dig() { 
  shove(); // shove is not valid unless cond2 or cond3 is set
           // (digger author didn't test cond1 without cond2 or cond3)
}
@k2d222
Copy link
Contributor

k2d222 commented Jan 22, 2025

I think it's reasonable to strip that code and expect no errors. But I think it's also reasonable to throw an error here. The eliminated code is arguably incorrect (both dig_hole and dig conditions should be cond1 && (cond2 || cond3), it is the programmer's mistake).

dead code elimination:

  • (+) avoids parsing virtually all the files, reduces drastically compilation time
  • (+) reduces drastically the output shader size
  • (-) does not catch errors in eliminated code (this example)
  • (-) does not include all const_asserts
  • (-) may be harder to implement (requires usage analysis)
  • (~) could be a feature if the programmer manages to enforce that code is always eliminated when it is invalid (this example, but I think it is bad practice)

So IMO DCE should be an optional feature of the linker, which may have side-effects when the code is incorrect.

I'm adding that it is possible for a linker or langserver to check that all condition combinations lead to valid code (at least that the referenced identifiers are declared), which mitigates the downside of DCE.

@mighdoll
Copy link
Contributor Author

Leaving DCE as linker now sounds good. Once the practical issues become clear, I suspect we'll want to pick a side on DCE for consistency / shader library compatibility... but maybe not and DCE stays implementation defined forever. I'll leave this issue open for now while we learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

No branches or pull requests

2 participants