David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Description • 7 years ago

It's painful to have to attach and check-in a 500KB patch every time we change a line of shader code. I'm pretty sure FXC is included on all Visual Studio versions we support, probably via the Windows SDK. We can pretty easily include it as a step in the build process.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 1 • 7 years ago

Mach can call out to external scripts to perform complex build steps, but the script must be Python. I could have just wrapped genshaders.sh in a Python script, but then the build wouldn't automatically recognize changes to included .hlsl files. So it made the most sense to just port the thing wholesale to Python.

Also, I'd like Advanced Layers to use the same script, so I separated the hardcoded shader list into a "shader.manifest" file. Since both CompositorD3D11 and Advanced Layers have multiple entry points per .hlsl file, the manifest format is designed to group entries in a similar way. For simplicity the manifest is in YAML.

Note 1: genshaders.py now wraps its output in the mozilla::layers namespace, so I had to fix up a few other files. (This is just for parity with AL's genshaders.sh file.)

Note 2: This patch provides a shell-only version of genshaders.py. Mach integration is in the next patch.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 2 • 7 years ago

This integrates genshaders.py into the build and deletes the pre-generated CompositorD3D11Shaders.h file currently checked into the tree. (Thus, the 450K patch.)

Luckily the shader compiler (FXC) has an option to dump #include use, so this patch makes use of that by parsing FXC's output, and returns the set of dependencies.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 3 • 7 years ago

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc12c00f9b3bb5352eb0ba625bf212e1c574f934

	Michael Shal [:mshal]
	Comment 4 • 7 years ago

Comment on attachment 8868910 [details] [diff] [review]
part 2, integrate into mach

>diff --git a/gfx/layers/d3d11/genshaders.py b/gfx/layers/d3d11/genshaders.py
>index a5eda7ebcc52..54ef26a7b714 100644
>--- a/gfx/layers/d3d11/genshaders.py
>+++ b/gfx/layers/d3d11/genshaders.py
>+def find_dependencies(fxc_output):
>+  # Dependencies look like this:
>+  #   Resolved to [<path>]
>+  #
>+  # Microsoft likes to change output strings based on the user's language, so
>+  # instead of pattern matching on that string, we take everything in between
>+  # brackets. We filter out potentially bogus strings later.
>+  deps = set()
>+  for line in fxc_output.split('\n'):
>+    m = re.search(r"\[([^\]]+)\]", line)
>+    if m is None:
>+      continue
>+    dep_path = m.group(1)
>+    dep_path = os.path.normpath(dep_path)
>+    if os.path.isfile(dep_path):
>+      deps.add(dep_path)
>+  return deps

Is this the only way to get dependencies out of fxc? It seems to work fine, but relying on the program output to be unchanged can be problematic if they decide to change the strings. I think in that case we would just silently lose the dependencies.

The generated CompositorD3D11Shaders.h file that I got had quite a big diff from the previous version checked into the tree. Is that intended? Seems to just be lots of random numbers, so I have no idea if it's significant :)

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 5 • 7 years ago

(In reply to Michael Shal [:mshal] from comment #4)
> Comment on attachment 8868910 [details] [diff] [review]
> 
> Is this the only way to get dependencies out of fxc? It seems to work fine,
> but relying on the program output to be unchanged can be problematic if they
> decide to change the strings. I think in that case we would just silently
> lose the dependencies.

Yeah unfortunately it's the only way. Is it a good idea to assert if no dependencies are found?

> The generated CompositorD3D11Shaders.h file that I got had quite a big diff
> from the previous version checked into the tree. Is that intended? Seems to
> just be lots of random numbers, so I have no idea if it's significant :)

I ran part 1 locally before doing the git rm, but didn't include those changes in the first patch. The order of the blobs changed which isn't significant. And fxc can output different blobs based on the compiler version, but it doesn't really matter.

	Bas Schouten (:bas.schouten)
	Comment 6 • 7 years ago

Comment on attachment 8868908 [details] [diff] [review]
part 1, rewrite genshaders in python

Review of attachment 8868908 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine to me. All machines we use now should use Python. Be sure to test this on VS2017.

	Michael Shal [:mshal]
	Comment 7 • 7 years ago

(In reply to David Anderson [:dvander] from comment #5)
> (In reply to Michael Shal [:mshal] from comment #4)
> > Comment on attachment 8868910 [details] [diff] [review]
> > 
> > Is this the only way to get dependencies out of fxc? It seems to work fine,
> > but relying on the program output to be unchanged can be problematic if they
> > decide to change the strings. I think in that case we would just silently
> > lose the dependencies.
> 
> Yeah unfortunately it's the only way. Is it a good idea to assert if no
> dependencies are found?

Sounds reasonable to me, if you always expect fxc to have some dependencies. Right now with only the one .hlsl file it just finds the same two includes so it should be fine.

Actually since this just uses #include like C code, another option would be to run cpp to get the dependencies (eg: 'cpp -M [shader_file]'). Though on Windows we have CPP as 'cl.exe -E', so maybe that won't be so easy.

	Pulsebot
	Comment 8 • 7 years ago

Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2323c4c38c31
Rewrite genshaders.sh as a Python script that uses a manifest. (bug 1365859 part 1, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5911d850652a
Generate shader blobs as part of the build process. (bug 1365859 part 2, r=mshal)

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 9 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2323c4c38c31
https://hg.mozilla.org/mozilla-central/rev/5911d850652a