SkSL performance improvements

SkSL performance improvements (plus a couple of minor warning fixes)
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2131223002

Committed: https://skia.googlesource.com/skia/+/9fd67a1f53809f5eff1210dd107241b450c48acc
Committed: https://skia.googlesource.com/skia/+/d598f7981f34811e6f2a949207dc13638852f3f7

[ethannicholas, 2016-07-08 14:40:41]

Description was changed from

==========
SkSL performance improvements
==========

to

==========
SkSL performance improvements
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2131223002
==========

[ethannicholas, 2016-07-08 15:00:24]

Patchset #2 (id:20001) has been deleted

[ethannicholas, 2016-07-08 15:00:34]

Patchset #1 (id:1) has been deleted

[ethannicholas, 2016-07-08 15:25:45]

Description was changed from

==========
SkSL performance improvements
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2131223002
==========

to

==========
SkSL performance improvements (plus a couple of minor warning fixes)
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2131223002
==========

[ethannicholas, 2016-07-08 15:26:27]

The CQ bit was checked by ethannicholas@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 15:26:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 15:34:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-08 15:34:06]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on
master.client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[ethannicholas, 2016-07-08 15:35:15]

ethannicholas@google.com changed reviewers:
+ benjaminwagner@google.com, egdaniel@google.com

[dogben, 2016-07-08 19:56:40]

LGTM, aside from the missing return.

general nit: It's a little scary to have a reference-typed field where the
constructor accepts the reference type, because you could accidentally store a
reference to a temporary, e.g.:

Variable makeVariable() { ... }

InterfaceBlock* segfaultyMakeInterfaceBlock(Position position) {
    return new InterfaceBlock(position, makeVariable());
}

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/SkSLIRGenerato...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/SkSLIRGenerato...
src/sksl/SkSLIRGenerator.cpp:435: delete decl;
Missing return.

nit: maybe stack-allocate the FunctionDeclaration in 'then' block and only set
decl in 'else' block.

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/SkSLSPIRVCodeG...
File src/sksl/SkSLSPIRVCodeGenerator.cpp (right):

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/SkSLSPIRVCodeG...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2369: Modifiers::kOut_Flag))) {
uniform OK to elide?

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/ir/SkSLFunctio...
File src/sksl/ir/SkSLFunctionDeclaration.h (right):

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/ir/SkSLFunctio...
src/sksl/ir/SkSLFunctionDeclaration.h:26: , fParameters(parameters)
nit: std::move

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/ir/SkSLFunctio...
src/sksl/ir/SkSLFunctionDeclaration.h:42: return fName == f.fName && fParameters
== f.fParameters;
(Not exactly related to this CL, but...) Seems like comparing parameters by
pointer== isn't really what you want here. Should this just check if the
parameter count and types are the same?

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/ir/SkSLSymbolT...
File src/sksl/ir/SkSLSymbolTable.cpp (right):

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/ir/SkSLSymbolT...
src/sksl/ir/SkSLSymbolTable.cpp:60: this->takeOwnership(result);
nit: Maybe takeOwnership could return the raw pointer so this could be "return
this->takeOwnership(result);"

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/ir/SkSLSymbolT...
src/sksl/ir/SkSLSymbolTable.cpp:98: for (const auto& f : ((UnresolvedFunction&)
*oldSymbol).fFunctions) {
nit: s/const auto&/const auto*/

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/ir/SkSLSymbolT...
File src/sksl/ir/SkSLSymbolTable.h (right):

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/ir/SkSLSymbolT...
src/sksl/ir/SkSLSymbolTable.h:26: : fOwner(true)
Would it be simpler for Block and FunctionDeclaration to own the SymbolTable
that owns their symbols? This would eliminate the need for fOwner. (We would
still need addWithoutOwnership for the global symbols and to add function
parameters to the function body block.)

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/ir/SkSLSymbolT...
src/sksl/ir/SkSLSymbolTable.h:40: void add(const std::string& name, Symbol*
symbol);
nit: use std::unique_ptr to transfer ownership

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/ir/SkSLSymbolT...
src/sksl/ir/SkSLSymbolTable.h:55: std::vector<Symbol*> fOwnedPointers;
nit: std::vector<std::unique_ptr<Symbol>> eliminates the destructor

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/ir/SkSLType.h
File src/sksl/ir/SkSLType.h (right):

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/ir/SkSLType.h#...
src/sksl/ir/SkSLType.h:207: std::vector<const Type*> coercibleTypes() const {
nit: return const ref

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/ir/SkSLVariable.h
File src/sksl/ir/SkSLVariable.h (right):

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/ir/SkSLVariabl...
src/sksl/ir/SkSLVariable.h:44: const std::string fValue;
nit: unused?

https://codereview.chromium.org/2131223002/diff/60001/src/sksl/ir/SkSLVariabl...
src/sksl/ir/SkSLVariable.h:58: struct hash<SkSL::Variable> {
(Not exactly related to this CL, but...) Is this used at all? It seems that
variables can't really be stored in hashmaps, since == doesn't really do what
you want. (Variable* is fine, though.)

[ethannicholas, 2016-07-11 19:06:37]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2016-07-11 19:06:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjaminwagner@google.com
Link to the patchset: https://codereview.chromium.org/2131223002/#ps80001
(title: "fixed nits")

[commit-bot: I haz the power, 2016-07-11 19:06:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 19:13:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-11 19:13:39]

Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[ethannicholas, 2016-07-11 19:21:00]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2016-07-11 19:21:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjaminwagner@google.com
Link to the patchset: https://codereview.chromium.org/2131223002/#ps100001
(title: "fixed windows build error")

[commit-bot: I haz the power, 2016-07-11 19:21:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 19:34:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-11 19:34:43]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on
master.client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[ethannicholas, 2016-07-11 19:56:59]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2016-07-11 19:57:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjaminwagner@google.com
Link to the patchset: https://codereview.chromium.org/2131223002/#ps120001
(title: "oops, got a bit overzealous")

[commit-bot: I haz the power, 2016-07-11 19:57:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dogben, 2016-07-11 20:05:33]

lgtm

So, parameters always go into the global symbol table? Does that still give an
error for something like:

void foo(int x) {
}

void main() {
  int y = x + x;
}

https://codereview.chromium.org/2131223002/diff/100001/src/sksl/ir/SkSLForSta...
File src/sksl/ir/SkSLForStatement.h (right):

https://codereview.chromium.org/2131223002/diff/100001/src/sksl/ir/SkSLForSta...
src/sksl/ir/SkSLForStatement.h:29: , fSymbols(symbols) {}
nit: std::move

https://codereview.chromium.org/2131223002/diff/100001/src/sksl/ir/SkSLInterf...
File src/sksl/ir/SkSLInterfaceBlock.h (right):

https://codereview.chromium.org/2131223002/diff/100001/src/sksl/ir/SkSLInterf...
src/sksl/ir/SkSLInterfaceBlock.h:30: , fTypeOwner(typeOwner) {
nit: std::move

https://codereview.chromium.org/2131223002/diff/100001/src/sksl/ir/SkSLSymbol...
File src/sksl/ir/SkSLSymbolTable.h (right):

https://codereview.chromium.org/2131223002/diff/100001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.h:40: Symbol* takeOwnership(Symbol* s);
nit: Sorry, I had meant to put the same comment here: use std::unique_ptr to
transfer ownership.

[commit-bot: I haz the power, 2016-07-11 20:07:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-11 20:07:02]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on
master.client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[ethannicholas, 2016-07-12 14:33:02]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2016-07-12 14:33:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjaminwagner@google.com
Link to the patchset: https://codereview.chromium.org/2131223002/#ps140001
(title: "went back to pointers on global types")

[commit-bot: I haz the power, 2016-07-12 14:33:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dogben, 2016-07-12 14:57:47]

lgtm

https://codereview.chromium.org/2131223002/diff/140001/src/sksl/ir/SkSLType.cpp
File src/sksl/ir/SkSLType.cpp (right):

https://codereview.chromium.org/2131223002/diff/140001/src/sksl/ir/SkSLType.c...
src/sksl/ir/SkSLType.cpp:171: const Type* kDMat2x2_Type = new Type("dmat2",  
*kFloat_Type, 2, 2);
Maybe I already asked this, but shouldn't the dmat types have a component type
of double?

[commit-bot: I haz the power, 2016-07-12 16:07:25]

Description was changed from

==========
SkSL performance improvements (plus a couple of minor warning fixes)
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2131223002
==========

to

==========
SkSL performance improvements (plus a couple of minor warning fixes)
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2131223002

Committed:
https://skia.googlesource.com/skia/+/9fd67a1f53809f5eff1210dd107241b450c48acc
==========

[commit-bot: I haz the power, 2016-07-12 16:07:26]

Committed patchset #6 (id:140001) as
https://skia.googlesource.com/skia/+/9fd67a1f53809f5eff1210dd107241b450c48acc

[msarett, 2016-07-13 19:51:15]

A revert of this CL (patchset #6 id:140001) has been created in
https://codereview.chromium.org/2143323003/ by msarett@google.com.

The reason for reverting is: Valgrind bot failures

https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GP....

[bungeman-skia, 2016-07-13 20:55:37]

bungeman@google.com changed reviewers:
+ bungeman@google.com

[bungeman-skia, 2016-07-13 20:55:38]

https://codereview.chromium.org/2131223002/diff/140001/src/sksl/ir/SkSLType.cpp
File src/sksl/ir/SkSLType.cpp (right):

https://codereview.chromium.org/2131223002/diff/140001/src/sksl/ir/SkSLType.c...
src/sksl/ir/SkSLType.cpp:134: const Type* kVoid_Type = new Type("void");
Also, if you ever want this to run in Chromium, these static initializers will
keep you out, as Chromium checks the number of static initializers (on the
'sizes' bot) and they don't let the number get any bigger. Is it possible to
make all of this constexpr?

[ethannicholas, 2016-07-14 14:34:49]

Description was changed from

==========
SkSL performance improvements (plus a couple of minor warning fixes)
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2131223002

Committed:
https://skia.googlesource.com/skia/+/9fd67a1f53809f5eff1210dd107241b450c48acc
==========

to

==========
SkSL performance improvements (plus a couple of minor warning fixes)
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2131223002

Committed:
https://skia.googlesource.com/skia/+/9fd67a1f53809f5eff1210dd107241b450c48acc
==========

[ethannicholas, 2016-07-14 19:55:05]

Patchset #7 (id:160001) has been deleted

[ethannicholas, 2016-07-14 19:57:19]

The CQ bit was checked by ethannicholas@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 19:57:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 20:28:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-14 20:28:47]

Dry run: This issue passed the CQ dry run.

[ethannicholas, 2016-07-14 20:51:34]

Patchset #8 (id:200001) has been deleted

[ethannicholas, 2016-07-14 20:57:26]

Does this look more palatable?

[dogben, 2016-07-14 21:30:04]

lgtm

I'm sorry that you have to code in C++.

https://codereview.chromium.org/2131223002/diff/180001/src/sksl/SkSLContext.h
File src/sksl/SkSLContext.h (right):

https://codereview.chromium.org/2131223002/diff/180001/src/sksl/SkSLContext.h...
src/sksl/SkSLContext.h:15: static std::vector<const Type*> type(const Type& t) {
nit: maybe make this a static method of Context, since it has such a generic
name.

[ethannicholas, 2016-07-25 16:44:58]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2016-07-25 16:44:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjaminwagner@google.com
Link to the patchset: https://codereview.chromium.org/2131223002/#ps220001
(title: " ")

[commit-bot: I haz the power, 2016-07-25 16:45:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 17:08:57]

Description was changed from

==========
SkSL performance improvements (plus a couple of minor warning fixes)
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2131223002

Committed:
https://skia.googlesource.com/skia/+/9fd67a1f53809f5eff1210dd107241b450c48acc
==========

to

==========
SkSL performance improvements (plus a couple of minor warning fixes)
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2131223002

Committed:
https://skia.googlesource.com/skia/+/9fd67a1f53809f5eff1210dd107241b450c48acc
Committed:
https://skia.googlesource.com/skia/+/d598f7981f34811e6f2a949207dc13638852f3f7
==========

[commit-bot: I haz the power, 2016-07-25 17:08:59]

Committed patchset #8 (id:220001) as
https://skia.googlesource.com/skia/+/d598f7981f34811e6f2a949207dc13638852f3f7