Add -O, -Os and -Oconfig flags. by dnovillo · Pull Request #817 · KhronosGroup/SPIRV-Tools

dnovillo · 2017-09-13T02:18:37Z

NOTE: This is still WIP. I wanted to provide an early patch to give an idea of where I intend to go with this. I would appreciate feedback on the direction this is going, so I can adjust it accordingly.

Major missing bits:

1- Proper expansion for -O. I added some transformations mostly to have something to debug,
but I am sure that list is incomplete and/or makes little sense.
2- Similarly for -Os.
3- Unit tests
4- There is some debugging code that still needs to be taken out.

======================================================================

These flags are expanded to a series of spirv-opt flags with the
following semantics:

-O: expands to passes that attempt to improve the performance of the
generated code.

-Os: expands to passes that attempt to reduce the size of the generated
code.

-Oconfig= expands to the sequence of passes determined by the
flags specified in the user-provided file.

greg-lunarg

I have commented on the documentation and content of the -O and -Os options. I leave infrastructure and style to those that are closer to those considerations.

greg-lunarg · 2017-09-13T16:46:05Z

tools/opt/size-passes.h

+
+namespace spvtools {
+
+// Size optimization flags expanded by -Os.


I would recommend at least these as a first approximation to what we want to include for size reduction:

--inline-entry-points-exhaustive
--convert-local-access-chains
--eliminate-local-single-block
--eliminate-local-single-store
--eliminate-insert-extract
--eliminate-dead-code-aggressive
--eliminate-dead-branches
--merge-blocks
--eliminate-local-multi-store
--eliminate-insert-extract
--eliminate-dead-code-aggressive
--eliminate-common-uniform

Of course, the exhaustive inlining is controversial at best, but we really don't have any other inlining at the moment and the rest of the options are not as effective for most shaders with function calls without some inlining. If we feel we want to make -Os available now, then I would include exhaustive inlining.

If you haven't seen this yet, you may want to read this as it will give you an idea how these passes work and how they are designed to work together: https://www.lunarg.com/wp-content/uploads/2017/08/SPIR-V-Shader-Size-Reduction-Using-spirv-opt_v1.0.pdf

I haven't given this data point before about exhaustive inlining: In my work on clspv, I'm working on some codebases originating from the OpenCL world. Some of the code I've seen has a large handful of entry points in the same compilation units, and they call down into common functions. Exhaustive inlining for all entry points would increase size.

Thanks. I've added these passes for now. We'll be testing them internally and we'll see how they work. I'm particularly perplexed at the exhaustive inliner, but I don't really have a good idea of why it's having this effect.

greg-lunarg · 2017-09-13T16:48:16Z

tools/opt/size-passes.h

+// Using the -Os flag in spirv-opt's command line is equivalent to specifying
+// the following flags in this exact sequence.
+std::vector<std::string> SizePasses = {
+    "--strip-debug",


Not sure we should include this as some users may wish to reduce the size of their SPIR-V but keep debugging info in.

greg-lunarg · 2017-09-13T16:49:13Z

tools/opt/size-passes.h

+    "--eliminate-dead-code-aggressive",
+    "--eliminate-dead-branches",
+    "--merge-blocks",
+    "--flatten-decorations",


Pretty sure --flatten-decorations will increase size. I don't think we want to include it here.

Agree. Also, I don't know of any tools that produce grouped decorations. :-)

greg-lunarg · 2017-09-13T16:50:31Z

tools/opt/size-passes.h

+    "--eliminate-dead-branches",
+    "--merge-blocks",
+    "--flatten-decorations",
+    "--compact-ids"


Pretty sure this does not reduce size. We should probably remove it.

Definitely agree on this one. spirv-remap works hard to set Ids so they look similar-ish across modules.

Thanks, folks. But I really didn't put any thought in these strings. They're very literally options I just picked at random. I would like the dev community to give an initial list of flags you think might be good for -O and -Os. I very literally do not care what we put here for now. I don't have a good set of benchmarks to try it for now.

greg-lunarg · 2017-09-13T17:03:06Z

tools/opt/opt.cpp

               first as its only predecessor. Performed only on entry point
               call tree functions.
-  -h, --help   
+  -O


I would not advertise -O and -Os until we are fairly confident they are close to what we want. We want to make sure initial experience with these options is positive. If you follow my recommendations for what to include for them, then I would be fine with advertising here.

That make some sense, especially while you've got this as a work in progress. But we also need a way to converge: How do we know we have a good set?

Alternately, once we've settled on an initial sane config, you could advertise this as EXPERIMENTAL.

It should probably always be listed a subject to change from one version of the software to the next.

I think marking it [experimental] is the way to go. No point hiding it if we want users to try it out.

greg-lunarg · 2017-09-13T17:11:23Z

tools/opt/opt-passes.h

+// Using the -O flag in spirv-opt's command line is equivalent to specifying
+// the following flags in this exact sequence.
+std::vector<std::string> OptimizationPasses = {
+    "--inline-entry-points-opaque",


See my recommendations for -Os. I would include those here as well. They are designed to eliminate all loads and stores to function scope variable which is a performance win besides a size win.

I would not use opaque inlining here. It was not designed with performance in mind (and it doesn't even quite solve the problem it was originally designed for). I am currently writing a "default" inliner which I think might be more acceptable here, but until that is ready I think the exhaustive inliner is going to be our best bet. Without some form of inlining, the other size-reduction passes will be much less effective for shaders with function calls.

Yes, this list will change over time. It's always subject to tuning.
Suggest adding a comment saying so.

Exhaustive inlining is probably the right thing for graphics shaders since there's only one entry point.
That's true for GLSL, and HLSL compilation via Glslang and dxc.
On the othe rhand, a module-combiner tool/API is in code review.
In OpenCL it's common to have multiple entry points in the same module.

Thanks. As a starting point, I've made both lists identical for now. I've added comments stating that they're in flux.

dneto0

The feedback you asked for: High level design looks good to me.

I've got some lower level comments as well, embedded.

And yes, the various lists will need tuning. Should state that they are always subject to revision. Say so in the comments and any online help you provide.

dneto0 · 2017-09-26T19:42:22Z

include/spirv-tools/optimizer.hpp


+  // Registers passes that attempt to improve performance of generated code.
+  // The sequence of passes is defined in the PerformancePasses vector.
+  void RegisterPerformancePasses();


It's a small thing, but I'd prefer this to return a reference to *this. That way we can stack calls, e.g.

RegsiterPerformancePasses().RegisterPass(blah blah)

dneto0 · 2017-09-26T19:44:37Z

tools/opt/opt-passes.h

+// Using the -O flag in spirv-opt's command line is equivalent to specifying
+// the following flags in this exact sequence.
+std::vector<std::string> OptimizationPasses = {
+    "--inline-entry-points-opaque",


Yes, this list will change over time. It's always subject to tuning.
Suggest adding a comment saying so.

Exhaustive inlining is probably the right thing for graphics shaders since there's only one entry point.
That's true for GLSL, and HLSL compilation via Glslang and dxc.
On the othe rhand, a module-combiner tool/API is in code review.
In OpenCL it's common to have multiple entry points in the same module.

dneto0 · 2017-09-26T19:49:59Z

tools/opt/opt.cpp

 using namespace spvtools;

+std::string Flags2Str(const std::vector<std::string>& flags) {
+  std::string s;


Out of habit we tend to accumulate on a std::ostringstream and then call the .str() method get the final result.
That avoids quadratic reallocation (in theory), which matters if the vector is large.

dneto0 · 2017-09-26T19:53:25Z

tools/opt/opt.cpp

               first as its only predecessor. Performed only on entry point
               call tree functions.
-  -h, --help   
+  -O


That make some sense, especially while you've got this as a work in progress. But we also need a way to converge: How do we know we have a good set?

Alternately, once we've settled on an initial sane config, you could advertise this as EXPERIMENTAL.

It should probably always be listed a subject to change from one version of the software to the next.

dneto0 · 2017-09-26T19:53:42Z

tools/opt/opt.cpp

+  -Os
+               Optimize for size. Apply a sequence of transformations in an
+               attempt to minimize the size of the generated code. This flag
+               is equivalent to specifying the following flags: %s


Similarly should say this is subject to change from one revision of the code to the next.

dneto0 · 2017-09-26T20:01:15Z

tools/opt/opt.cpp

+    flags.insert(flags.end(), SizePasses.begin(), SizePasses.end());
+  } else if (0 == strncmp(opt_flag, "-Oconfig=", sizeof("-Oconfig=") - 1)) {
+    std::vector<std::string> file_flags;
+    if (!ReadFlagsFromFile(opt_flag, &file_flags)) {


So I guess -Oconfig=foo inside a flags file does not work? That's fine, I'm sure. :-)

Yeah, I was debating whether to allow it or not. I'm disallowing it for now, but we can review this later. It's possible to have -O and -Os inside the configuration file, of course. I figured a -Oconfig= inside a configuration file is just asking for trouble.

dneto0 · 2017-09-26T20:03:03Z

tools/opt/size-passes.h

+//
+// Using the -Os flag in spirv-opt's command line is equivalent to specifying
+// the following flags in this exact sequence.
+std::vector<std::string> SizePasses = {


Make a function to return this.

"Variables of class type with static storage duration are forbidden: they cause hard-to-find bugs due to indeterminate order of construction and destruction"

Done. I've reworked the whole -O and -Os handling. They are now part of the Optimizer API. There are two new methods: RegisterPerformancePasses and RegisterSizePasses. These do the direct instantiation of the related passes. Having them inside the Optimizer API allows us to invoke them from places other than spirv-opt.

dneto0 · 2017-09-26T20:03:53Z

tools/opt/size-passes.h

+    "--eliminate-dead-code-aggressive",
+    "--eliminate-dead-branches",
+    "--merge-blocks",
+    "--flatten-decorations",


Agree. Also, I don't know of any tools that produce grouped decorations. :-)

dneto0 · 2017-09-26T20:04:13Z

tools/opt/size-passes.h

+// Size optimization flags expanded by -Os.
+//
+// Using the -Os flag in spirv-opt's command line is equivalent to specifying
+// the following flags in this exact sequence.


Recommend a note this list is subject to continual tuning.

dneto0 · 2017-09-26T20:04:53Z

tools/opt/size-passes.h

+    "--eliminate-dead-branches",
+    "--merge-blocks",
+    "--flatten-decorations",
+    "--compact-ids"


Definitely agree on this one. spirv-remap works hard to set Ids so they look similar-ish across modules.

dneto0 · 2017-09-26T20:10:35Z

tools/opt/opt.cpp

+                    --eliminate-dead-code-aggressive program.spv
+
+               Note that the flags -O, -Os and -Oconfig=<file> are also
+               affected by other flags present on the command line. For


Can we improve the "affected" phrasing? How about:

Is this better?

The -O, -Os, and -Oconfig flags act as macros. Using one of them is equivalent to explicitly inserting the underlying flags at that position in the command line.

dneto0

High level concern: I wonder if the lists of passes be available via public API. That way an external tool (such as glslc) could reuse the list when performing the optimizations. That way we could keep the list (and tuning knowledge) in one place rather than replicating it.

dnovillo · 2017-10-05T14:46:09Z

I've addressed all the requested changes. PTAL. Thanks.

dneto0

Low level item: Method comment on GetPassNames should talk about lifetime of the char pointers.

High level item: The name advertised by a particular pass via its name() method is often different from what we use on the command line. So that will be confusing.
Two options:

Explain the difference. (Yucky)
Change the internal name to the public name (without leading double-hyphen). And forever catch a particular discrepancy in code review.

I prefer option 2. I don't think anybody really depends on the specific result of the name() method yet.

What do you think?

dneto0 · 2017-10-05T16:58:30Z

include/spirv-tools/optimizer.hpp

           std::vector<uint32_t>* optimized_binary) const;

+  // Returns a vector of strings with all the pass names added to this
+  // optimizer's pass manager.


Since they are const char* you should say how long the pointers are valid. They are valid until the pass manager is destroyed.

(Or, change it to return std::string)

These flags are expanded to a series of spirv-opt flags with the following semantics: -O: expands to passes that attempt to improve the performance of the generated code. -Os: expands to passes that attempt to reduce the size of the generated code. -Oconfig=<file> expands to the sequence of passes determined by the flags specified in the user-provided file.

dnovillo · 2017-10-06T16:46:57Z

On Thu, Oct 5, 2017 at 5:07 PM, David Neto ***@***.***> wrote: ***@***.**** requested changes on this pull request. Low level item: Method comment on GetPassNames should talk about lifetime of the char pointers. High level item: The name advertised by a particular pass via its name() method is often different from what we use on the command line. So that will be confusing. Two options: 1. Explain the difference. (Yucky) 2. Change the internal name to the public name (without leading double-hyphen). And forever catch a particular discrepancy in code review. I prefer option 2. I don't think anybody really depends on the specific result of the name() method yet. What do you think?

Yeah, that was bugging me too but I forgot to talk about it in my response earlier. I prefer #2 too. We could also set the name at pass construction time. The driver would pass these strings directly from the flags. But I think it's easier to give them a fixed string name and catch it in code review. I've changed all the passes to advertise their spirv-opt flag name.

------------------------------ In include/spirv-tools/optimizer.hpp <#817 (comment)> : > // // It's allowed to alias |original_binary| to the start of |optimized_binary|. bool Run(const uint32_t* original_binary, size_t original_binary_size, std::vector<uint32_t>* optimized_binary) const; + // Returns a vector of strings with all the pass names added to this + // optimizer's pass manager. Since they are const char* you should say how long the pointers are valid. They are valid until the pass manager is destroyed. (Or, change it to return std::string)

Done. No point converting to std::string since they come from the const char * runtime pool. PTAL. Thanks.

dneto0 · 2017-10-10T16:08:36Z

source/opt/pass.h

+  // NOTE: When deriving a new pass class, make sure you make the name
+  // compatible with the corresponding spirv-opt command-line flag. For example,
+  // if you add the flag --my-pass to spirv-opt, make this function return
+  // "my-pass" (no leading hyphens).


dneto0 · 2017-10-10T16:15:32Z

Rebased and pushed into master as c90d730

Roll third_party/glslang/ 973d0e5..7bc0473 (1 commit) KhronosGroup/glslang@973d0e5...7bc0473 $ git log 973d0e5..7bc0473 --date=short --no-merges --format='%ad %ae %s' 2019-09-18 laddoc Reflection will crash when the VS input symbol defines the same name with FS output symbol Roll third_party/googletest/ f2fb48c3b..dc1ca9ae4 (7 commits) google/googletest@f2fb48c...dc1ca9a $ git log f2fb48c3b..dc1ca9ae4 --date=short --no-merges --format='%ad %ae %s' 2019-09-29 misterg Googletest export 2019-09-27 misterg Googletest export 2019-09-25 absl-team Googletest export 2019-09-25 absl-team Googletest export 2019-09-24 absl-team Googletest export 2019-09-19 absl-team Googletest export 2019-09-27 gennadiycivil Bump llvm version to 4 so brew can work again Roll third_party/spirv-headers/ 601d738..842ec90 (4 commits) KhronosGroup/SPIRV-Headers@601d738...842ec90 $ git log 601d738..842ec90 --date=short --no-merges --format='%ad %ae %s' 2019-09-24 ehsannas Improve the doc on using Bazel. 2019-09-24 rex.xu Bump the SPIR-V version to 1.5 2019-09-23 ehsannas Update documentation. 2019-09-18 ehsannas Add a Bazel build file. Roll third_party/spirv-tools/ f62ee4a..9eb1c9a (22 commits) KhronosGroup/SPIRV-Tools@f62ee4a...9eb1c9a $ git log f62ee4a..9eb1c9a --date=short --no-merges --format='%ad %ae %s' 2019-10-01 stevenperron Add continue construct analysis to struct cfg analysis (KhronosGroup#2922) 2019-09-27 stevenperron Record trailing line dbg instructions (KhronosGroup#2926) 2019-09-27 rharrison Add removing references to debug instructions when removing them (KhronosGroup#2923) 2019-09-27 paulthomson spirv-fuzz: allow interestingness script arguments (KhronosGroup#2925) 2019-09-27 ehsannas Add Kokoro bots for building with Bazel. (KhronosGroup#2914) 2019-09-27 alanbaker Refactor the InstructionPass (KhronosGroup#2924) 2019-09-26 afdx spirv-fuzz: do not allow a dead break to target an unreachable block (KhronosGroup#2917) 2019-09-26 afdx spirv-fuzz: preserve some analyses when permuting blocks (KhronosGroup#2918) 2019-09-25 alanbaker Only allow previously declared forward refs in structs (KhronosGroup#2920) 2019-09-25 stevenperron Handle id overflow in wrap-opkill (KhronosGroup#2916) 2019-09-25 afdx spirv-fuzz: do not replace struct indices with synonyms (KhronosGroup#2915) 2019-09-25 afdx spirv-fuzz: Fixes to preconditions for adding dead break/continue edges (KhronosGroup#2904) 2019-09-25 afdx spirv-fuzz: do not replace a pointer argument to a function call with a synonym (KhronosGroup#2901) 2019-09-25 afdx spirv-fuzz: do not replace boolean constant argument to OpPhi instruction (KhronosGroup#2903) 2019-09-24 alanbaker Remove validate_datarules.cpp (KhronosGroup#2911) 2019-09-24 stevenperron Handle extract with no indexes (KhronosGroup#2910) 2019-09-24 ehsannas Add Bazel build configuration. (KhronosGroup#2891) 2019-09-24 stevenperron Handle id overflow in convert local access chains (KhronosGroup#2908) 2019-09-24 dsinclair Add OpCopyMemory test to SVA. (KhronosGroup#2885) 2019-09-23 dsinclair Add missing GN dependency (KhronosGroup#2899) 2019-09-23 afdx Employ the "swarm testing" idea in spirv-fuzz (KhronosGroup#2890) 2019-09-23 afdx Fix operand index in spirv-fuzz (KhronosGroup#2895) Created with: roll-dep third_party/effcee third_party/glslang third_party/googletest third_party/re2 third_party/spirv-cross third_party/spirv-headers third_party/spirv-tools

greg-lunarg suggested changes Sep 13, 2017

View reviewed changes

dneto0 changed the title ~~Add -O, -Os and -Oconfig flags.~~ WIP: Add -O, -Os and -Oconfig flags. Sep 20, 2017

dneto0 requested changes Sep 26, 2017

View reviewed changes

dneto0 reviewed Sep 26, 2017

View reviewed changes

dneto0 mentioned this pull request Sep 26, 2017

Add support for more optimization passes from SPIRV-Tools google/shaderc#373

Open

dneto0 reviewed Sep 27, 2017

View reviewed changes

dnovillo changed the title ~~WIP: Add -O, -Os and -Oconfig flags.~~ Add -O, -Os and -Oconfig flags. Sep 29, 2017

dnovillo force-pushed the opt-pipeline branch 4 times, most recently from 2e0a7fb to d544bdb Compare October 5, 2017 14:45

dneto0 requested changes Oct 5, 2017

View reviewed changes

dnovillo force-pushed the opt-pipeline branch from d544bdb to e501dec Compare October 6, 2017 16:46

dneto0 approved these changes Oct 10, 2017

View reviewed changes

dneto0 closed this Oct 10, 2017

dnovillo deleted the opt-pipeline branch October 17, 2017 18:56


		namespace spvtools {

		// Size optimization flags expanded by -Os.

Conversation

dnovillo commented Sep 13, 2017

Uh oh!

greg-lunarg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dneto0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnovillo Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dneto0 left a comment

Choose a reason for hiding this comment

Uh oh!

dnovillo Oct 3, 2017 •

edited

Loading