initial checkin of SkSL compiler

initial checkin of SkSL compiler

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1984363002
CQ_EXTRA_TRYBOTS=client.skia.compile:Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot,Build-Mac-Clang-x86_64-Release-CMake-Trybot

Committed: https://skia.googlesource.com/skia/+/b3058bdb1049ca75d526eb9f11e1a42a49e63585

[ethannicholas, 2016-05-17 17:41:58]

Description was changed from

==========
initial checkin of SkSL compiler

BUG=skia:
==========

to

==========
initial checkin of SkSL compiler

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[ethannicholas, 2016-05-17 18:40:47]

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

[commit-bot: I haz the power, 2016-05-17 18:40:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984363002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984363002/40001

[commit-bot: I haz the power, 2016-05-17 18:45:09]

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

[commit-bot: I haz the power, 2016-05-17 18:45:10]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

[djsollen, 2016-06-10 14:07:23]

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

[djsollen, 2016-06-10 14:07:25]

https://codereview.chromium.org/1984363002/diff/180001/src/gpu/vk/GrVkGpu.h
File src/gpu/vk/GrVkGpu.h (right):

https://codereview.chromium.org/1984363002/diff/180001/src/gpu/vk/GrVkGpu.h#n...
src/gpu/vk/GrVkGpu.h:11: #define USE_SKSL 0
shouldn't this be 1?

[ethannicholas, 2016-06-14 18:11:20]

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

[commit-bot: I haz the power, 2016-06-14 18:11:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984363002/260001

[commit-bot: I haz the power, 2016-06-14 18:23:33]

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

[commit-bot: I haz the power, 2016-06-14 18:23:34]

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

[ethannicholas, 2016-06-14 18:57:49]

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

[commit-bot: I haz the power, 2016-06-14 18:57:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984363002/280001

[commit-bot: I haz the power, 2016-06-14 19:03:31]

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

[commit-bot: I haz the power, 2016-06-14 19:03:33]

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

[ethannicholas, 2016-06-14 19:14:15]

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

[commit-bot: I haz the power, 2016-06-14 19:14:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984363002/300001

[commit-bot: I haz the power, 2016-06-14 19:29:12]

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

[commit-bot: I haz the power, 2016-06-14 19:29:14]

Dry run: This issue passed the CQ dry run.

[ethannicholas, 2016-06-17 15:35:11]

ethannicholas@google.com changed reviewers:
+ csmartdalton@google.com, dogben@google.com, jvanverth@google.com,
mtklein@google.com, stephana@google.com

[ethannicholas, 2016-06-17 15:35:13]

A few things to keep in mind as you look at this:

STD: To make it easier to use the compiler outside of Skia, it deliberately
minimizes its usage of Skia APIs, e.g. using std::string rather than SkString
and so forth. Mike and Brian both indicated that they are ok with this at least
for the time being.

TESTING: Currently there are some negative tests (more to come later), but no
positive tests. Since merely running gm under Vulkan ends up compiling thousands
of shaders, Skia ends up doing a ton of positive testing itself. I am aware that
this is inadequate, but am hoping it is enough for an initial checkin. I am
currently working on a "real" test framework for the compiler, but it won't be
ready for a bit and I'd prefer to go ahead and get this pushed without further
delay.

FLEX OUTPUT: lex.skiasl.c is generated from skia.flex. Normally I am opposed to
putting generated files under source control, but the lexer will not change
often and this avoids the requirement of installing flex on all of the build
machines; any objections?

[dogben, 2016-06-20 16:23:12]

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

[dogben, 2016-06-20 16:23:19]

I've reviewed the AST code and figured I would send my comments so far. Please
feel free to completely ignore any comment labeled "nit."

I skimmed the parser code, but I'm a bit lost. Is there a language description
or code examples I can read first?

https://codereview.chromium.org/1984363002/diff/320001/src/gpu/vk/GrVkPipelin...
File src/gpu/vk/GrVkPipelineStateBuilder.cpp (right):

https://codereview.chromium.org/1984363002/diff/320001/src/gpu/vk/GrVkPipelin...
src/gpu/vk/GrVkPipelineStateBuilder.cpp:103: std::stringstream buffer;
Unused?

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTBi...
File src/sksl/ast/SkSLASTBinaryExpression.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTBi...
src/sksl/ast/SkSLASTBinaryExpression.h:15: namespace SkSL {
nit: maybe "namespace AST" instead of prefixing all the type names with "AST"?

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTBl...
File src/sksl/ast/SkSLASTBlock.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTBl...
src/sksl/ast/SkSLASTBlock.h:24: std::string result("{");
uber-nit: prefer = initialization. Here's way more discussion on the topic than
you want: https://herbsutter.com/2013/05/09/gotw-1-solution/

(Same pattern in other files.)

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTDe...
File src/sksl/ast/SkSLASTDeclaration.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTDe...
src/sksl/ast/SkSLASTDeclaration.h:19: enum Kind {
nit: In the context of a compiler, "Kind" seems like a loaded word, i.e.:
https://en.wikipedia.org/wiki/Kind_(type_theory)

OTOH, maybe it's the least loaded of the loaded words?

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTDi...
File src/sksl/ast/SkSLASTDiscardStatement.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTDi...
src/sksl/ast/SkSLASTDiscardStatement.h:23: return std::string("discard;");
uber-nit: "std::string(" unnecessary.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTDo...
File src/sksl/ast/SkSLASTDoStatement.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTDo...
src/sksl/ast/SkSLASTDoStatement.h:19: ASTDoStatement(Position position,
std::unique_ptr<ASTExpression> test,
nit: should test and statement be in the opposite order? (Same for fields
below.)

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTEx...
File src/sksl/ast/SkSLASTExtension.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTEx...
src/sksl/ast/SkSLASTExtension.h:21: , fName(name) {}
nit: std::move

(Same comment in other files.)

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFl...
File src/sksl/ast/SkSLASTFloatLiteral.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFl...
src/sksl/ast/SkSLASTFloatLiteral.h:24: return to_string(fValue);
nit: "std::to_string"

(Same comment in other files.)

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFo...
File src/sksl/ast/SkSLASTForStatement.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFo...
src/sksl/ast/SkSLASTForStatement.h:20: std::unique_ptr<ASTExpression> test,
std::unique_ptr<ASTExpression> next,
Should next be a statement?

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFo...
src/sksl/ast/SkSLASTForStatement.h:29: std::string result = std::string("for
(");
uber-nit: "std::string(" unnecessary

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFo...
src/sksl/ast/SkSLASTForStatement.h:31:
result.append(fInitializer->description());
uber-nit: elsewhere in this CL, you use += rather than .append.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFo...
src/sksl/ast/SkSLASTForStatement.h:33: result.append(" ");
;

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFo...
src/sksl/ast/SkSLASTForStatement.h:41: return result + ") " +
fStatement->description();
nit: Is the compiler smart enough to append to result and return it, or does
this make a copy of result?

(Same comment in other files.)

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTIf...
File src/sksl/ast/SkSLASTIfStatement.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTIf...
src/sksl/ast/SkSLASTIfStatement.h:31: if (fIfFalse != nullptr) {
nit: "if (fIfFalse) {" seems clearer to me.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTIn...
File src/sksl/ast/SkSLASTInterfaceBlock.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTIn...
src/sksl/ast/SkSLASTInterfaceBlock.h:22: */
nit: mention that fValueName can be empty.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTIn...
src/sksl/ast/SkSLASTInterfaceBlock.h:25: ASTModifiers modifiers,
nit: seems inconsistent that this is not std::unique_ptr.

(FWIW, I think I would generally prefer that everything is by value.)

(Same comment in other files.)

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTLa...
File src/sksl/ast/SkSLASTLayout.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTLa...
src/sksl/ast/SkSLASTLayout.h:20: */
nit: mention -1 means missing.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTNo...
File src/sksl/ast/SkSLASTNode.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTNo...
src/sksl/ast/SkSLASTNode.h:21: virtual ~ASTNode() {}
nit: wrong indentation

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTPa...
File src/sksl/ast/SkSLASTParameter.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTPa...
src/sksl/ast/SkSLASTParameter.h:28: , fSizes(sizes) {}
nit: std::move

(Same comment in other files.)

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTPa...
src/sksl/ast/SkSLASTParameter.h:31: return fModifiers.description() +
fType->description() + " " + fName;
fSizes not included?

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTRe...
File src/sksl/ast/SkSLASTReturnStatement.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTRe...
src/sksl/ast/SkSLASTReturnStatement.h:17: */
nit: mention that expression can be null.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTSu...
File src/sksl/ast/SkSLASTSuffix.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTSu...
src/sksl/ast/SkSLASTSuffix.h:17: * Abstract supertype of all expression
suffixes, such as '[0]' or '.rgb'. Suffixes are not
nit: "Abstract" is not quite true, since this is instantiated for ++/--.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTTe...
File src/sksl/ast/SkSLASTTernaryExpression.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTTe...
src/sksl/ast/SkSLASTTernaryExpression.h:20: std::unique_ptr<ASTExpression>
ifTrue,
nit: wrong indentation

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTTy...
File src/sksl/ast/SkSLASTType.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTTy...
src/sksl/ast/SkSLASTType.h:14: * A type, such as 'int' or 'struct foo { ... }'.
Is this abstract? I don't quite see how it can represent structs.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTVa...
File src/sksl/ast/SkSLASTVarDeclaration.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTVa...
src/sksl/ast/SkSLASTVarDeclaration.h:28:
std::vector<std::vector<std::unique_ptr<ASTExpression>>> sizes,
Just to make sure I understand: array variable declarations can have dynamic
sizes, but array parameters have static sizes.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTVa...
src/sksl/ast/SkSLASTVarDeclaration.h:53: }
missing comma between variables?

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTVa...
File src/sksl/ast/SkSLASTVarDeclarationStatement.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTVa...
src/sksl/ast/SkSLASTVarDeclarationStatement.h:17: * A variable declaration
appearing as a statement within a function.
Maybe it doesn't matter, but is the final semicolon part of ASTVarDeclaration or
part of ASTVarDeclarationStatement?

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTWh...
File src/sksl/ast/SkSLASTWhileStatement.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTWh...
src/sksl/ast/SkSLASTWhileStatement.h:20: std::unique_ptr<ASTStatement>
statement)
nit: wrong indentation

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/sksl.flex
File src/sksl/sksl.flex (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/sksl.flex#new...
src/sksl/sksl.flex:121: "||" { return SkSL::Token::LOGICALOR; }
I don't know flex well enough to know if this will match when BITWISEOR is
listed before LOGICALOR.

[ethannicholas, 2016-06-20 17:45:50]

Updated in response to Ben's comments. Ben also asked for some information
regarding the parser:

The language we are parsing is called GLSL, which is an extremely C-like
language. The grammar is documented in the parser comments above each production
(e.g. the comment on the block() function reads "LBRACE statement* RBRACE",
indicating that it is a left brace, any number of statements, and a right
brace). You should be able to find tons of documentation and examples online.

"SkSL" will eventually exist as language in its own right, but for the moment
the compiler is only worrying about GLSL.

https://codereview.chromium.org/1984363002/diff/320001/src/gpu/vk/GrVkPipelin...
File src/gpu/vk/GrVkPipelineStateBuilder.cpp (right):

https://codereview.chromium.org/1984363002/diff/320001/src/gpu/vk/GrVkPipelin...
src/gpu/vk/GrVkPipelineStateBuilder.cpp:103: std::stringstream buffer;
On 2016/06/20 16:23:17, Ben Wagner wrote:
> Unused?

A bit of debug code I failed to remove. Oops!

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTBi...
File src/sksl/ast/SkSLASTBinaryExpression.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTBi...
src/sksl/ast/SkSLASTBinaryExpression.h:15: namespace SkSL {
On 2016/06/20 16:23:17, Ben Wagner wrote:
> nit: maybe "namespace AST" instead of prefixing all the type names with "AST"?

Normally I would, but in this case there are so many similarly- or
identically-named classes that I would get confused without the prefix.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTDe...
File src/sksl/ast/SkSLASTDeclaration.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTDe...
src/sksl/ast/SkSLASTDeclaration.h:19: enum Kind {
On 2016/06/20 16:23:17, Ben Wagner wrote:
> nit: In the context of a compiler, "Kind" seems like a loaded word, i.e.:
> https://en.wikipedia.org/wiki/Kind_(type_theory)
> 
> OTOH, maybe it's the least loaded of the loaded words?

"Type" is even more overloaded :-). Is there a better word to represent "What
kind of declaration is this?"

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFl...
File src/sksl/ast/SkSLASTFloatLiteral.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFl...
src/sksl/ast/SkSLASTFloatLiteral.h:24: return to_string(fValue);
On 2016/06/20 16:23:18, Ben Wagner wrote:
> nit: "std::to_string"
> 
> (Same comment in other files.)

std::to_string isn't present on some Android versions. This is a call to an SkSL
function.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFo...
File src/sksl/ast/SkSLASTForStatement.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFo...
src/sksl/ast/SkSLASTForStatement.h:20: std::unique_ptr<ASTExpression> test,
std::unique_ptr<ASTExpression> next,
On 2016/06/20 16:23:18, Ben Wagner wrote:
> Should next be a statement?

This is for the third entry in a for loop ('for (int i = 0; i < 10; i++)'); only
expressions are legal there.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTIn...
File src/sksl/ast/SkSLASTInterfaceBlock.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTIn...
src/sksl/ast/SkSLASTInterfaceBlock.h:25: ASTModifiers modifiers,
On 2016/06/20 16:23:18, Ben Wagner wrote:
> nit: seems inconsistent that this is not std::unique_ptr.
> 
> (FWIW, I think I would generally prefer that everything is by value.)
> 
> (Same comment in other files.)

Statements and expressions are both recursive structures, so I can't use by
value on those. Modifiers are one of the few cases that are both simple enough
to pass by value and that I don't have any reason to care about the identity of.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTTy...
File src/sksl/ast/SkSLASTType.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTTy...
src/sksl/ast/SkSLASTType.h:14: * A type, such as 'int' or 'struct foo { ... }'.
On 2016/06/20 16:23:18, Ben Wagner wrote:
> Is this abstract? I don't quite see how it can represent structs.

The comment was slightly inaccurate -- it doesn't represent the full structure,
rather just the "struct foo" part. The rest is represented in a Type.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTVa...
File src/sksl/ast/SkSLASTVarDeclaration.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTVa...
src/sksl/ast/SkSLASTVarDeclaration.h:28:
std::vector<std::vector<std::unique_ptr<ASTExpression>>> sizes,
On 2016/06/20 16:23:18, Ben Wagner wrote:
> Just to make sure I understand: array variable declarations can have dynamic
> sizes, but array parameters have static sizes.

Correct.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTVa...
File src/sksl/ast/SkSLASTVarDeclarationStatement.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTVa...
src/sksl/ast/SkSLASTVarDeclarationStatement.h:17: * A variable declaration
appearing as a statement within a function.
On 2016/06/20 16:23:19, Ben Wagner wrote:
> Maybe it doesn't matter, but is the final semicolon part of ASTVarDeclaration
or
> part of ASTVarDeclarationStatement?

Should have been part of ASTVarDeclarationStatement. All of the description()
functions are only used for debugging purposes, so I wasn't as careful with them
as I should have been. Fixed.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/sksl.flex
File src/sksl/sksl.flex (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/sksl.flex#new...
src/sksl/sksl.flex:121: "||" { return SkSL::Token::LOGICALOR; }
On 2016/06/20 16:23:19, Ben Wagner wrote:
> I don't know flex well enough to know if this will match when BITWISEOR is
> listed before LOGICALOR.

The order only matters when the matched text is the same length.

[mtklein, 2016-06-20 18:25:25]

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFo...
File src/sksl/ast/SkSLASTForStatement.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFo...
src/sksl/ast/SkSLASTForStatement.h:20: std::unique_ptr<ASTExpression> test,
std::unique_ptr<ASTExpression> next,
On 2016/06/20 17:45:49, ethannicholas wrote:
> On 2016/06/20 16:23:18, Ben Wagner wrote:
> > Should next be a statement?
> 
> This is for the third entry in a for loop ('for (int i = 0; i < 10; i++)');
only
> expressions are legal there.

It helps me to remember that many things you think of as statements (like
assignment) are actually expressions.  IIRC, the for-loop grammar just allows
two forms, with a special case for starting with a declaration:
   for (expr; expr; expr)
   for (decl; expr; expr)

[dogben, 2016-06-20 18:26:26]

Replying to some comments, but that forces publishing all comments. :-)

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/SkSLToken.h
File src/sksl/SkSLToken.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/SkSLToken.h#n...
src/sksl/SkSLToken.h:11: #include "ast/SkSLASTPositionNode.h"
should be SkSLPosition.h?

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/SkSLToken.h#n...
src/sksl/SkSLToken.h:126: case Token::SHLEQ:        return "<<";
missing = for the next 5

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/SkSLToken.h#n...
src/sksl/SkSLToken.h:148: , fText(text) {}
nit: std::move

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/SkSLUtil.h
File src/sksl/SkSLUtil.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/SkSLUtil.h#ne...
src/sksl/SkSLUtil.h:47: #define ASSERT_RESULT(x) { bool result = x; ASSERT(x); }
How is this different from ASSERT? Why does it evaluate x twice?

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTDe...
File src/sksl/ast/SkSLASTDeclaration.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTDe...
src/sksl/ast/SkSLASTDeclaration.h:19: enum Kind {
On 2016/06/20 at 17:45:49, ethannicholas wrote:
> On 2016/06/20 16:23:17, Ben Wagner wrote:
> > nit: In the context of a compiler, "Kind" seems like a loaded word, i.e.:
> > https://en.wikipedia.org/wiki/Kind_(type_theory)
> > 
> > OTOH, maybe it's the least loaded of the loaded words?
> 
> "Type" is even more overloaded :-). Is there a better word to represent "What
kind of declaration is this?"

Where you use "Kind" to enumerate subclasses, I would probably just use "Enum,"
but I can understand if you don't like that option. My next choice would be
"Sort," but I'm not sure that's better than "Kind."

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFl...
File src/sksl/ast/SkSLASTFloatLiteral.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFl...
src/sksl/ast/SkSLASTFloatLiteral.h:24: return to_string(fValue);
On 2016/06/20 at 17:45:49, ethannicholas wrote:
> On 2016/06/20 16:23:18, Ben Wagner wrote:
> > nit: "std::to_string"
> > 
> > (Same comment in other files.)
> 
> std::to_string isn't present on some Android versions. This is a call to an
SkSL function.

Sorry! I haven't gotten used to the "don't include anything unless you have to"
Skia style.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFo...
File src/sksl/ast/SkSLASTForStatement.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTFo...
src/sksl/ast/SkSLASTForStatement.h:20: std::unique_ptr<ASTExpression> test,
std::unique_ptr<ASTExpression> next,
On 2016/06/20 at 17:45:49, ethannicholas wrote:
> On 2016/06/20 16:23:18, Ben Wagner wrote:
> > Should next be a statement?
> 
> This is for the third entry in a for loop ('for (int i = 0; i < 10; i++)');
only expressions are legal there.

Ok, I see that in the parser, e.g. "i += 2" becomes an ASTExpression rather than
an ASTStatement.

https://codereview.chromium.org/1984363002/diff/320001/tests/SkSLErrorTest.cpp
File tests/SkSLErrorTest.cpp (right):

https://codereview.chromium.org/1984363002/diff/320001/tests/SkSLErrorTest.cp...
tests/SkSLErrorTest.cpp:13: SkSL::Compiler compiler;
nit: Given that these tests are very similar, putting the boilerplate in a
function would make them easier to read. E.g.:
DEF_TEST(SkSLUndefinedSymbol, r) {
    TestFragmentError(r,
                      "void main() { x = vec2(1); }",
                      "error: 1: unknown identifier 'x'\n1 error\n");
}

https://codereview.chromium.org/1984363002/diff/320001/tests/SkSLErrorTest.cp...
tests/SkSLErrorTest.cpp:15: compiler.errorText();
unused?

https://codereview.chromium.org/1984363002/diff/320001/tests/SkSLErrorTest.cp...
tests/SkSLErrorTest.cpp:21: std::string("error: 1: unknown identifier 'x'\n1
error\n"));
nit: == can compare std::string and char*, so "std::string(" is unnecessary.

(Many instances in this file.)

https://codereview.chromium.org/1984363002/diff/320001/tests/SkSLErrorTest.cp...
tests/SkSLErrorTest.cpp:55: std::string("error: 1: call to 'foo' expected 1
parameter, but found 2\n"
nit: maybe add \n to the code to test the line number in error message?

https://codereview.chromium.org/1984363002/diff/320001/tests/SkSLErrorTest.cp...
tests/SkSLErrorTest.cpp:140: REPORTER_ASSERT(r, result);
There is no error when constructing vec3 with 4 values?

[ethannicholas, 2016-06-20 20:17:10]

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/SkSLUtil.h
File src/sksl/SkSLUtil.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/SkSLUtil.h#ne...
src/sksl/SkSLUtil.h:47: #define ASSERT_RESULT(x) { bool result = x; ASSERT(x); }
On 2016/06/20 18:26:26, Ben Wagner wrote:
> How is this different from ASSERT? Why does it evaluate x twice?

Because I'm a moron, that's why.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTDe...
File src/sksl/ast/SkSLASTDeclaration.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/ast/SkSLASTDe...
src/sksl/ast/SkSLASTDeclaration.h:19: enum Kind {
On 2016/06/20 18:26:26, Ben Wagner wrote:
> On 2016/06/20 at 17:45:49, ethannicholas wrote:
> > On 2016/06/20 16:23:17, Ben Wagner wrote:
> > > nit: In the context of a compiler, "Kind" seems like a loaded word, i.e.:
> > > https://en.wikipedia.org/wiki/Kind_(type_theory)
> > > 
> > > OTOH, maybe it's the least loaded of the loaded words?
> > 
> > "Type" is even more overloaded :-). Is there a better word to represent
"What
> kind of declaration is this?"
> 
> Where you use "Kind" to enumerate subclasses, I would probably just use
"Enum,"
> but I can understand if you don't like that option. My next choice would be
> "Sort," but I'm not sure that's better than "Kind."

Unless you've got a major objection, I'd prefer to just stick with Kind. While
I'm aware of the type theory meaning of the word, "enum Enum" and "enum Sort"
both seem to me to have bigger problems :-).

https://codereview.chromium.org/1984363002/diff/320001/tests/SkSLErrorTest.cpp
File tests/SkSLErrorTest.cpp (right):

https://codereview.chromium.org/1984363002/diff/320001/tests/SkSLErrorTest.cp...
tests/SkSLErrorTest.cpp:13: SkSL::Compiler compiler;
On 2016/06/20 18:26:26, Ben Wagner wrote:
> nit: Given that these tests are very similar, putting the boilerplate in a
> function would make them easier to read. E.g.:
> DEF_TEST(SkSLUndefinedSymbol, r) {
>     TestFragmentError(r,
>                       "void main() { x = vec2(1); }",
>                       "error: 1: unknown identifier 'x'\n1 error\n");
> }

Crap, I had meant to go do that. Fixed.

https://codereview.chromium.org/1984363002/diff/320001/tests/SkSLErrorTest.cp...
tests/SkSLErrorTest.cpp:15: compiler.errorText();
On 2016/06/20 18:26:26, Ben Wagner wrote:
> unused?

Once upon a time, calling errorText() returned the text and cleared it, so I was
using this to clear it. I cleaned up the API and failed to remove this (now
unnecessary) call to it.

https://codereview.chromium.org/1984363002/diff/320001/tests/SkSLErrorTest.cp...
tests/SkSLErrorTest.cpp:140: REPORTER_ASSERT(r, result);
On 2016/06/20 18:26:26, Ben Wagner wrote:
> There is no error when constructing vec3 with 4 values?

Stupid, huh? For whatever reason, it's supposed to be legal (and Skia actually
does this in at least one spot).

[dogben, 2016-06-21 17:53:48]

Sending comments on reviewing the parser.

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/SkSLUtil.h
File src/sksl/SkSLUtil.h (right):

https://codereview.chromium.org/1984363002/diff/320001/src/sksl/SkSLUtil.h#ne...
src/sksl/SkSLUtil.h:47: #define ASSERT_RESULT(x) { bool result = x; ASSERT(x); }
On 2016/06/20 20:17:10, ethannicholas wrote:
> On 2016/06/20 18:26:26, Ben Wagner wrote:
> > How is this different from ASSERT? Why does it evaluate x twice?
> 
> Because I'm a moron, that's why.

For non-DEBUG, I see how these are different. I'm still not understanding
ASSERT_RESULT for DEBUG. I only see one use of ASSERT_RESULT, which seems like
it could be just ASSERT.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cpp
File src/sksl/SkSLParser.cpp (right):

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:58: : fPushback(Position(-1, -1), Token::INVALID_TOKEN,
std::string(""))
nit: remove "std::string("

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:59: , fTypes(types)
nit: std::move

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:88: fPushback = t;
nit: maybe ASSERT(fPushback.fKind == Token::INVALID_TOKEN)?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:105: next.fText + "'");
nit: odd indentation

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:119: case Token::END_OF_FILE:
nit: inconsistent use of braces

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:134: return result;
Any reason this isn't continue?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:149: case Token::LOWP: // fall through
If it's intentional that the precision is ignored, could you add a comment?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:172: return nullptr;
ignored?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:185: return std::unique_ptr<ASTDeclaration>(new
ASTExtension(start.fPosition, name.fText));
nit (very optional): std::move; many places in this file, where Token is a
local.

(Normally I wouldn't suggest to use std::move on a field, but my understanding
is that this compiler should be blazingly fast, and avoiding string copies is an
easy way to improve speed.)

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:205: if (type->fKind == ASTType::kStruct_Kind &&
peek().fKind == Token::SEMICOLON) {
ISTM this case is not mentioned in the method doc. Maybe I'm just
lost-in-silly-parentheses, but it's not a function or interface, and doesn't
match "modifiers type IDENTIFIER SEMICOLON" because we're not parsing names.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:210: return std::unique_ptr<ASTVarDeclaration>(new
ASTVarDeclaration(std::move(modifiers),
This case represents a struct declaration with no variables, correct? It might
be worthwhile adding comments:
 - In ASTVarDeclaration, note that it's also used for struct type decls with no
var declarations.
 - Here, maybe note that the variable names are optional when declaring a
struct.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:212: names,
nit: std::move?

Or maybe just pass {} for the empty vectors so that no temporary objects are
created?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:216: Token name;
Should we check that name is not a reserved word?

Same question for other declarations. Maybe we should have a version of
identifier() that is used for declarations?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:249: return this->varDeclarationEnd(modifiers,
std::move(type), name.fText);
nit: Might be easier to read if funcDeclarationEnd is also a separate method.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:254: std::unique_ptr<ASTType>
Parser::structDeclaration() {
nit: order of declarations in .h doesn't match the order of definitions in .cpp

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:259: if (!this->expect(Token::IDENTIFIER, "an
identifier", &name)) {
nit: If I'm reading the GLSL spec correctly, the type name is optional
(presumably in the case of a variable declaration). However, I'm very much fine
with not supporting that case if it's not used in Skia.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:274: for (int j = (int) decl->fSizes[i].size() - 1; j >=
0; j--) {
j is iterating backwards, so I think that means "int arr[5][6]" will be an array
of length 5 of arrays of length 6 of ints, so (presumably) the memory location
of arr[x][y] will be arr + 5*x + y. Does that match your intention?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:275: if (decl->fSizes[i][j]->fKind ==
ASTExpression::kInt_Kind) {
Should this be !=?

I don't see any tests that involve structs.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:277: }
Missing return?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:286: }
Missing return?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:301: std::unique_ptr<ASTType> type = this->type();
Maybe check that type is not a struct declaration?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:313: if (!this->expect(Token::INT_LITERAL, "a
non-negative integer", &sizeToken)) {
nit: "a positive integer"?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:322: std::move(type), name.fText, sizes));
nit: std::move(sizes)

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:449: std::vector<std::unique_ptr<ASTStatement>>()));
nit: s/std::vector<std::unique_ptr<ASTStatement>>()/{}/

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:503: }
Missing return?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:520: name.fText, valueName,
nit: std::move

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:539: else {
nit: "} else {"

Two places.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:604: names,
nit: std::move

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:620: return
this->varDeclarationEnd(std::move(modifiers), std::move(type), name.fText);
nit: copy- and move-constructors are equally efficient for ASTModifiers.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:872: return this->assignmentExpression();
nit: The spec says there's a sequence operator, but I'm completely fine with
omitting it, assuming it's not needed for "for" loops.

(By "the spec" I mean the PDF that I get when I Google "glsl spec":
https://www.opengl.org/registry/doc/GLSLangSpec.4.40.pdf )

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:905: result = std::unique_ptr<ASTExpression>(new
ASTBinaryExpression(std::move(result),
Isn't this left-associative rather than right-associative? "x = y = 3" would be
parsed as:
result = "x"
result = "(x) = y"
result = "((x) = y) = 3"

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:915: /* logicalOrExpression ('?' expression ':'
assignmentExpression)? */
How should this be parsed?
"a ? b : c = d"
My understanding is that the spec says it should be parsed as ((a ? b : c) = d),
but it also says selection is disallowed as an l-value. Maybe this is a case of
"the compiler knows what I mean and should just do it," it which case what
you've implemented is fine.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:972: /* bitwiseXorExpression (LOGICALAND
bitwiseXorExpression)* */
Should be or, not xor, throughout this method.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:1219: this->expect(Token::RBRACKET, "']' to complete
array access expression");
Maybe a feature for another CL: add method expectClose(Token open, Token::Kind
close) that generates a message like "Expected ']' at 33:16 to match '[' at
33:13"

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:1226: return std::unique_ptr<ASTSuffix>(new
ASTFieldSuffix(pos, text));
nit: std::move(text)

(Also other callsites of identifier.)

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:1265: std::unique_ptr<ASTExpression> result;
nit: Seems clearer to return immediately rather than assign to result and return
at the end.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.h
File src/sksl/SkSLParser.h (right):

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.h#...
src/sksl/SkSLParser.h:50: Parser(std::string text, std::shared_ptr<SymbolTable>
types, ErrorReporter& errors);
nit: Would "const std::string& text" work here?

nit: In what circumstances would the caller have SymbolTable go out of scope
before Parser? (If never, it seems like raw pointer (or reference, if you
prefer) would make more sense than shared_ptr.)

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.h#...
src/sksl/SkSLParser.h:163: * an intervening nextToken()).
nit: Maybe also note that you can't call pushback after peek?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.h#...
src/sksl/SkSLParser.h:191: void* fScanner;
nit: Any reason not to make fields and helper methods private?

[dogben, 2016-06-21 21:52:52]

I've reviewed SymbolTable and the Symbol class hierarchy.

I think in the long run you will be happier if you don't use std::shared_ptr. It
seems that it wouldn't be too hard to change your code so that SymbolTable owns
all Symbols, and keep the SymbolTable around for the entire compile step. Then
you could change all the std::shared_ptr to just raw pointers, with the
understanding that all Symbols are owned by SymbolTable.

std::shared_ptr is pretty heavy-weight. IIUC, every construction, assignment,
reset, or destruction causes a memory barrier. It also has a memory overhead.
IMO, it's worth some extra effort to avoid using it.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLErrorRepo...
File src/sksl/SkSLErrorReporter.h (right):

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLErrorRepo...
src/sksl/SkSLErrorReporter.h:20: virtual ~ErrorReporter() {}
odd indentation

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLExpres...
File src/sksl/ir/SkSLExpression.h (right):

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLExpres...
src/sksl/ir/SkSLExpression.h:41: , fType(type) {}
std::move

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLFuncti...
File src/sksl/ir/SkSLFunctionDeclaration.h (right):

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLFuncti...
src/sksl/ir/SkSLFunctionDeclaration.h:30: std::string result =
fReturnType->fName + " " + fName + "(";
nit: maybe fReturnType->description()?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLFuncti...
src/sksl/ir/SkSLFunctionDeclaration.h:32: for (auto p : fParameters) {
nit: const ref

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLFuncti...
src/sksl/ir/SkSLFunctionDeclaration.h:41: bool
matches(std::shared_ptr<FunctionDeclaration> f) {
Should this method be const?

nit: parameter can be const ref

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLFuncti...
src/sksl/ir/SkSLFunctionDeclaration.h:45: mutable bool fDefined;
nit: Seems like this doesn't really need to be mutable.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLSymbol...
File src/sksl/ir/SkSLSymbolTable.cpp (right):

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.cpp:16:
result.push_back(std::static_pointer_cast<FunctionDeclaration>(s));
nit: return {std::static_pointer_cast<FunctionDeclaration>(s)};

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.cpp:19: for (auto f :
std::static_pointer_cast<UnresolvedFunction>(s)->fFunctions) {
nit: return std::static_pointer_cast<UnresolvedFunction>(s)->fFunctions;

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

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.h:19:
std::vector<std::shared_ptr<FunctionDeclaration>>
get_functions(std::shared_ptr<Symbol> s);
nit: make this a static method of SymbolTable?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.h:34: std::shared_ptr<Symbol> operator[](std::string
name) {
Can this return const ref?

IIUC, every creation and destruction of std::shared_ptr is a memory barrier, so
it's really worth the trouble to eliminate as many copies as possible.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.h:35: auto entry = fSymbols.find(name);
nit: const ref

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.h:47: auto previousFunctions =
get_functions(entry->second);
Should this be get_functions(previous)?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.h:48: for (std::shared_ptr<FunctionDeclaration> prev
: previousFunctions) {
nit: const ref

Next loop also.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.h:73: fSymbols[name] = symbol;
nit: std::move(symbol), everywhere in this function.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.h:75: std::shared_ptr<Symbol> oldSymbol =
existing->second;
nit: const ref?

Or regular ref and use .reset below?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.h:77:
std::vector<std::shared_ptr<FunctionDeclaration>> functions;
nit: Construct with capacity 2?

Or use {} initialization?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.h:80: fSymbols[name] = std::shared_ptr<Symbol>(new
UnresolvedFunction(functions));
nit: std::move, two places

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.h:82:
std::vector<std::shared_ptr<FunctionDeclaration>> functions;
nit: Construct with capacity
std::static_pointer_cast<UnresolvedFunction>(oldSymbol)->fFunctions.size()?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.h:83: for (auto f :
std::static_pointer_cast<UnresolvedFunction>(oldSymbol)->fFunctions) {
nit: const auto&

Or:
const auto& oldFunctions =
std::static_pointer_cast<UnresolvedFunction>(oldSymbol)->fFunctions;
functions.insert(functions.end(), oldFunctions.begin(), oldFunctions.end());

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.h:88: }
else error?

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

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.c...
src/sksl/ir/SkSLType.cpp:131: ABORT("unsupported scalar_to_compound type %s",
this->description().c_str());
bool not needed?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.c...
src/sksl/ir/SkSLType.cpp:232: result.push_back(t1);
nit: std::move x4

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.c...
src/sksl/ir/SkSLType.cpp:239: const std::shared_ptr<Type> kGSampler1D_Type(new
Type("$gsampler1D", type(kSampler1D_Type)));
I don't understand this. We're passing the result of type to fCoercibleTypes.
type gives me the same type 4 times. Why would we want the same type 4 times on
the coercion list?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.c...
src/sksl/ir/SkSLType.cpp:244: type(kSampler2DRect_Type)));
nit: odd indentation, until genType

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.c...
src/sksl/ir/SkSLType.cpp:275: const std::shared_ptr<Type> kVec_Type(new
Type("$vec", types(kVec2_Type, kVec2_Type, kVec3_Type,
kVec2_Type twice is correct?

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

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:29: , fName(name)
nit: std::move

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:30: , fType(type) {}
nit: std::move

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:52: Type(std::string name)
nit: add documentation of what each constructor means?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:53: : INHERITED(Position(), kType_Kind, name)
nit: std::move, many places in this file

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:136: return fName == other.fName;
Isn't the following legal?
struct same {
  int x;
} a;

void f () {
  struct same {
    float y;
  } b;
}

It seems that just comparing name is not sufficient.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:209: * For generic types, teturns the types that this
generic type can substitute for. For other
typo teturns

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:246: size_t alignment() {
I don't understand a few of these calculations.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:250: case kVector_Kind: // fall through
no fall through

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:263: ABORT("unsupported array type");
What about multi-dimensional arrays?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:293: return 16;
Is this correct for kDMat4x4_Type? Seems like it should be 32 (4 components * 8
bytes per component).

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:295: return 16;
Shouldn't this be based on the component size and align?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:315: return fColumns * this->alignment();
Shouldn't this be fColumns * (fComponentType->size() rounded up to the next
fComponentType->alignment())?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:323: ASSERT(offset % alignment == 0);
I don't see offset defined.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:342: Kind fKind;
nit: This shadows Symbol::fKind.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLUnreso...
File src/sksl/ir/SkSLUnresolvedFunction.h (right):

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLUnreso...
src/sksl/ir/SkSLUnresolvedFunction.h:21: , fFunctions(funcs) {
nit: std::move

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLUnreso...
src/sksl/ir/SkSLUnresolvedFunction.h:22: for (auto func : funcs) {
nit: const ref

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLVariab...
File src/sksl/ir/SkSLVariable.h (right):

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLVariab...
src/sksl/ir/SkSLVariable.h:34: , fType(type)
nit: std::move

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLVariab...
src/sksl/ir/SkSLVariable.h:44: const std::string fValue;
unused?

[dogben, 2016-06-21 21:54:46]

On 2016/06/21 at 21:52:52, Ben Wagner wrote:
> I've reviewed SymbolTable and the Symbol class hierarchy.
> 
> I think in the long run you will be happier if you don't use std::shared_ptr.
It seems that it wouldn't be too hard to change your code so that SymbolTable
owns all Symbols, and keep the SymbolTable around for the entire compile step.
Then you could change all the std::shared_ptr to just raw pointers, with the
understanding that all Symbols are owned by SymbolTable.
> 
> std::shared_ptr is pretty heavy-weight. IIUC, every construction, assignment,
reset, or destruction causes a memory barrier. It also has a memory overhead.
IMO, it's worth some extra effort to avoid using it.

Forgot to mention: if you get rid of std::shared_ptr, you can ignore most of my
comments about std::move or using const references.

[dogben, 2016-06-21 21:58:00]

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLVariab...
File src/sksl/ir/SkSLVariable.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLVariab...
src/sksl/ir/SkSLVariable.h:48: mutable bool fIsReadFrom;
nit: Seems like fIsReadFrom and fIsWrittenTo don't need to be mutable.

[ethannicholas, 2016-06-22 16:06:44]

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cpp
File src/sksl/SkSLParser.cpp (right):

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:134: return result;
On 2016/06/21 17:53:48, Ben Wagner wrote:
> Any reason this isn't continue?

The parser doesn't make any attempts at error recovery yet, so I was just
bailing rather than interpreting the rest of the file as garbage. However, I've
gone ahead and changed it so I don't have to return a dummy value from
declaration() (see below).

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:210: return std::unique_ptr<ASTVarDeclaration>(new
ASTVarDeclaration(std::move(modifiers),
On 2016/06/21 17:53:46, Ben Wagner wrote:
> This case represents a struct declaration with no variables, correct? It might
> be worthwhile adding comments:
>  - In ASTVarDeclaration, note that it's also used for struct type decls with
no
> var declarations.
>  - Here, maybe note that the variable names are optional when declaring a
> struct.

There should have been a comment here explaining that I was just returning a
dummy value to avoid returning null, but in any case I've changed it to simply
return null now.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:216: Token name;
On 2016/06/21 17:53:46, Ben Wagner wrote:
> Should we check that name is not a reserved word?
> 
> Same question for other declarations. Maybe we should have a version of
> identifier() that is used for declarations?

Token::IDENTIFIER is never a reserved word, by definition. That's handled by the
lexer.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:277: }
On 2016/06/21 17:53:47, Ben Wagner wrote:
> Missing return?

The lack of return was deliberate -- we've ignored the (invalid) expression, but
we still know where we are in the parse and can keep going just fine.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:286: }
On 2016/06/21 17:53:46, Ben Wagner wrote:
> Missing return?

Same as above.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:301: std::unique_ptr<ASTType> type = this->type();
On 2016/06/21 17:53:47, Ben Wagner wrote:
> Maybe check that type is not a struct declaration?

I've updated type() to only handle non-struct-declaration types.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:313: if (!this->expect(Token::INT_LITERAL, "a
non-negative integer", &sizeToken)) {
On 2016/06/21 17:53:48, Ben Wagner wrote:
> nit: "a positive integer"?

You're right, zero doesn't make sense here either.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:449: std::vector<std::unique_ptr<ASTStatement>>()));
On 2016/06/21 17:53:46, Ben Wagner wrote:
> nit: s/std::vector<std::unique_ptr<ASTStatement>>()/{}/

You know, I've gotten dinged in previous reviews for using the short syntax. I'm
not completely clear on what our coding standard is :-).

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:872: return this->assignmentExpression();
On 2016/06/21 17:53:46, Ben Wagner wrote:
> nit: The spec says there's a sequence operator, but I'm completely fine with
> omitting it, assuming it's not needed for "for" loops.
> 
> (By "the spec" I mean the PDF that I get when I Google "glsl spec":
> https://www.opengl.org/registry/doc/GLSLangSpec.4.40.pdf )

The omission was deliberate; I've never been a huge fan of the comma operator in
the first place, and neither Skia nor any shader I've encountered in the wild
use it.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:905: result = std::unique_ptr<ASTExpression>(new
ASTBinaryExpression(std::move(result),
On 2016/06/21 17:53:47, Ben Wagner wrote:
> Isn't this left-associative rather than right-associative? "x = y = 3" would
be
> parsed as:
> result = "x"
> result = "(x) = y"
> result = "((x) = y) = 3"

No. When faced with x = y = 3, we first parse a ternaryExpression. This results
in "x", since "=" cannot be part of a ternaryExpression. We then grab the equals
and finally an assignmentExpression, which results in "x = (y = 3)".

[dogben, 2016-06-22 17:43:58]

Responses to previous comments and comments on the IR below. Next up, the
IRGenerator!

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cpp
File src/sksl/SkSLParser.cpp (right):

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:216: Token name;
On 2016/06/22 at 16:06:44, ethannicholas wrote:
> On 2016/06/21 17:53:46, Ben Wagner wrote:
> > Should we check that name is not a reserved word?
> > 
> > Same question for other declarations. Maybe we should have a version of
> > identifier() that is used for declarations?
> 
> Token::IDENTIFIER is never a reserved word, by definition. That's handled by
the lexer.

IIUC, the lexer will prevent tokens like "while" or "return" from being
identifiers. According to the spec, it's not allowed to redefine a number of
built-ins, such as "vec2" or "sampler1D." There are also some future reserved
words, like "typedef" and "partition".

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:274: for (int j = (int) decl->fSizes[i].size() - 1; j >=
0; j--) {
On 2016/06/21 at 17:53:46, Ben Wagner wrote:
> j is iterating backwards, so I think that means "int arr[5][6]" will be an
array of length 5 of arrays of length 6 of ints, so (presumably) the memory
location of arr[x][y] will be arr + 5*x + y. Does that match your intention?

I read the spec section on arrays, and it matches what you've implemented. Very
confusing, but I guess it makes sense that arrays and matrices are treated
similarly.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:449: std::vector<std::unique_ptr<ASTStatement>>()));
On 2016/06/22 at 16:06:44, ethannicholas wrote:
> On 2016/06/21 17:53:46, Ben Wagner wrote:
> > nit: s/std::vector<std::unique_ptr<ASTStatement>>()/{}/
> 
> You know, I've gotten dinged in previous reviews for using the short syntax.
I'm not completely clear on what our coding standard is :-).

It's definitely a matter of taste/style. Like auto, if it's not obvious to the
reader what type it will end up being, it's probably better to be explicit. IMO,
"new ASTBlock(start.fPosition, {})" is obviously an empty block.

One thing that's never a good idea is "auto x = {...};". Generally, there's no
good reason to have a variable of type initializer_list.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLBlock.h
File src/sksl/ir/SkSLBlock.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLBlock....
src/sksl/ir/SkSLBlock.h:29: result += "\n}\n";
nit: final newline unnecessary?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLBreakS...
File src/sksl/ir/SkSLBreakStatement.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLBreakS...
src/sksl/ir/SkSLBreakStatement.h:19: struct BreakStatement : public Statement {
In my experience, breaks and continues disappear when converting AST to IR. But
maybe it's useful to keep around if you are targeting GLSL.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLConstr...
File src/sksl/ir/SkSLConstructor.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLConstr...
src/sksl/ir/SkSLConstructor.h:20: std::vector<std::unique_ptr<Expression>>
parameters)
nit: s/parameters/arguments/g

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLConstr...
src/sksl/ir/SkSLConstructor.h:21: : INHERITED(position, kConstructor_Kind, type)
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLConstr...
src/sksl/ir/SkSLConstructor.h:24: virtual std::string description() const
override {
nit: remove virtual

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLDoStat...
File src/sksl/ir/SkSLDoStatement.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLDoStat...
src/sksl/ir/SkSLDoStatement.h:19: struct DoStatement : public Statement {
Similar to comment above, in my experience, loops disappear in IR.

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

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLForSta...
src/sksl/ir/SkSLForStatement.h:34: result += " ";
;

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLFuncti...
File src/sksl/ir/SkSLFunctionCall.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLFuncti...
src/sksl/ir/SkSLFunctionCall.h:20: FunctionCall(Position position,
std::shared_ptr<FunctionDeclaration> function,
Checking my understanding: Reading the GLSL spec, it sounds like functions can
only be called by name. Even the "subroutine" thing looks like a function
declaration. So, things like "(b ? f1 : f2)(arg1, arg2)" are not allowed.
Correct?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLFuncti...
src/sksl/ir/SkSLFunctionCall.h:21: std::vector<std::unique_ptr<Expression>>
parameters)
nit: s/parameters/arguments/g

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLFuncti...
src/sksl/ir/SkSLFunctionCall.h:23: , fFunction(function)
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLFuncti...
src/sksl/ir/SkSLFunctionCall.h:26: virtual std::string description() const
override {
nit: remove virtual

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLFuncti...
File src/sksl/ir/SkSLFunctionDeclaration.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLFuncti...
src/sksl/ir/SkSLFunctionDeclaration.h:32: for (auto p : fParameters) {
nit: const ref

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLFuncti...
src/sksl/ir/SkSLFunctionDeclaration.h:45: mutable bool fDefined;
nit: seems like this doesn't need to be mutable.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLFuncti...
File src/sksl/ir/SkSLFunctionDefinition.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLFuncti...
src/sksl/ir/SkSLFunctionDefinition.h:24: , fDeclaration(declaration)
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLFuncti...
File src/sksl/ir/SkSLFunctionReference.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLFuncti...
src/sksl/ir/SkSLFunctionReference.h:17: * always eventually replaced by
FunctionCalls in valid programs.
nit: s/FunctionCall/FunctionDeclaration/?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLFuncti...
src/sksl/ir/SkSLFunctionReference.h:22: , fFunctions(function) {}
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLIndexE...
File src/sksl/ir/SkSLIndexExpression.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLIndexE...
src/sksl/ir/SkSLIndexExpression.h:17: * Given a type, returns the type that will
resulting from extracting an array value from it.
typo: s/resulting/result/

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLIndexE...
src/sksl/ir/SkSLIndexExpression.h:52: virtual std::string description() const
override {
nit: remove virtual

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

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLInterf...
src/sksl/ir/SkSLInterfaceBlock.h:29: , fVariable(var) {
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLLayout.h
File src/sksl/ir/SkSLLayout.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLLayout...
src/sksl/ir/SkSLLayout.h:57: result = "layout (" + result + ")";
nit: just return?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLProgram.h
File src/sksl/ir/SkSLProgram.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLProgra...
src/sksl/ir/SkSLProgram.h:22: enum Kind {
nit: wrong indentation

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLSwizzle.h
File src/sksl/ir/SkSLSwizzle.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLSwizzl...
src/sksl/ir/SkSLSwizzle.h:21: static std::shared_ptr<Type> get_type(Expression&
value,
nit: I'm used to all references being const, but I don't know what style Skia
normally uses.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLSwizzl...
src/sksl/ir/SkSLSwizzle.h:29: case 1: return kFloat_Type;
nit: Aren't all the "case 1"s in this function redundant?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLSwizzl...
src/sksl/ir/SkSLSwizzle.h:73: , fComponents(components) {
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLSwizzl...
src/sksl/ir/SkSLSwizzle.h:77: virtual std::string description() const override {
nit: remove virtual

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLTernar...
File src/sksl/ir/SkSLTernaryExpression.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLTernar...
src/sksl/ir/SkSLTernaryExpression.h:21: std::unique_ptr<Expression> ifTrue,
std::unique_ptr<Expression> ifFalse)
nit: odd indentation

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLTernar...
src/sksl/ir/SkSLTernaryExpression.h:29: virtual std::string description() const
override {
nit: remove virtual

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

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:32: std::string description() {
override? const?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:217: int dimensions() {
nit: should this and all following methods be const?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLTypeRe...
File src/sksl/ir/SkSLTypeReference.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLTypeRe...
src/sksl/ir/SkSLTypeReference.h:22: , fValue(type) {}
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLTypeRe...
src/sksl/ir/SkSLTypeReference.h:24: virtual std::string description() const
override {
nit: remove virtual

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLVarDec...
File src/sksl/ir/SkSLVarDeclaration.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLVarDec...
src/sksl/ir/SkSLVarDeclaration.h:18: * A variable declaration, which may consist
of multiple individual variables. For instance
Is there any reason not to split the multiple variable case into separate
individual VarDeclaration objects for IR? Seems like that would make things
simpler.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLVariab...
File src/sksl/ir/SkSLVariableReference.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLVariab...
src/sksl/ir/SkSLVariableReference.h:24: : INHERITED(position,
kVariableReference_Kind, variable->fType)
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLVariab...
src/sksl/ir/SkSLVariableReference.h:25: , fVariable(variable) {}
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLVariab...
src/sksl/ir/SkSLVariableReference.h:27: virtual std::string description() const
override {
nit: remove virtual

[dogben, 2016-06-23 15:25:12]

Comments below on IRGenerator.

Is there a design doc for SkSL? I'd like to understand a bit more about the
goals and which targets we may support in the future.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:70: : fSymbolTable(symbolTable)
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:75: fSymbolTable.reset(new
SymbolTable(fSymbolTable, fErrors));
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:94: case ASTStatement:: kIf_Kind:
nit: extra space

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:110: default:
nit: Do we compile with the warning enabled when not all values of the enum are
handled?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:129: ASTVarDeclarationStatement& s) {
nit: odd indentation

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:138: return Modifiers(modifiers);
Shouldn't we check that modifiers don't conflict?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:151: Modifiers modifiers =
this->convertModifiers(decl.fModifiers);
nit: could be moved outside the loop?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:153: std::vector<std::unique_ptr<Expression>>
currentVarSizes;
If baseType is an array type, should the sizes be added to currentVarSizes
before iterating over fSizes?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:164: count = ((IntLiteral&) *size).fValue;
Check that count is positive?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:167: count = -1;
Check that size's type is integer?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:173: currentVarSizes.push_back(nullptr);
Don't we need to update type here also?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:177: auto var = std::shared_ptr<Variable>(new
Variable(decl.fPosition, modifiers,
nit: std::make_shared

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:188: value = nullptr;
nit: redundant

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:191: fSymbolTable->add(var->fName, var);
nit: move this line before variables.push_back?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:193: return std::unique_ptr<VarDeclaration>(new
VarDeclaration(decl.fPosition, variables,
nit: std::move(variables)

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:227: std::unique_ptr<Expression> next =
this->convertExpression(*f.fNext);
Check that next is not FunctionReference or TypeReference?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:254: std::unique_ptr<Expression> test =
this->convertExpression(*d.fTest);
Other tests are coerced. Why not this one?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:276: return std::unique_ptr<Statement>(new
ExpressionStatement(std::move(e)));
Check that e is not FunctionReference or TypeReference?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:285: ASSERT(fCurrentFunction);
nit: Move outside of if?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:316: static std::shared_ptr<Type>
expand_generics(std::shared_ptr<Type> type, int i) {
nit: This function should probably accept and return Type* rather than
shared_ptr<Type>.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:323: static void
expand_generics(std::shared_ptr<FunctionDeclaration> decl,
nit: This function is not taking ownership of its arguments, so it should accept
raw pointers rather than shared_ptr. Same comment for many of the
functions/methods in this file. Unless the callee retains a reference after it
returns, it shouldn't accept std::shared_ptr as an argument.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:328: for (auto p : decl->fParameters) {
nit: const ref

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:329:
parameters.push_back(std::shared_ptr<Variable>(new Variable(decl->fPosition,
Should this be p->fPosition?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:334: Variable::kParameter_Storage)));
nit: odd indentation

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:338: parameters,
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:339: returnType));
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:352: isGeneric = returnType->kind() ==
Type::kGeneric_Kind;
nit: declare and initialize in one statement.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:354: for (size_t i = 0; i < f.fParameters.size();
i++) {
nit: could use range-based for

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:362: Type::kArray_Kind, type, size));
nit: std::move(type)

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:369: name,
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:370: type,
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:371: Variable::kParameter_Storage));
nit: odd indentation

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:372: parameters.push_back(var);
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:392: for (auto other : functions) {
nit: const ref

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:419: fSymbolTable->add(parameters[i]->fName,
decl->fParameters[i]);
Are modifiers allowed to differ between declaration and definition (like const
in C++)?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:429: for (auto var : parameters) {
nit: const ref

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:442: std::unique_ptr<Block> body =
this->convertBlock(*f.fBody);
Seems like a very minor issue, but the spec says "A function's parameter
declarations and body together form a single scope nested in the global scope,"
and "...a declared name cannot be redeclared in the same scope..."

convertBlock uses AutoSymbolTable to start a new scope, so the function body is
in a different scope from the parameters, and therefore a local var could shadow
a parameter.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:461: Variable::kGlobal_Storage);
nit: odd indentation

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:463:
fields.push_back(Type::Field(decl->fVars[j]->fModifiers, decl->fVars[j]->fName,
Shouldn't we OR in mods.fFlags? And check that they don't conflict?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:475: Variable::kGlobal_Storage));
nit: odd indentation

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:535: f));
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:546: var));
nit: std::move

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:556: return std::unique_ptr<TypeReference>(new
TypeReference(identifier.fPosition, t));
nit: std::move(t)

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:569: if (*expr->fType == *type) {
nit: Elsewhere, you compare types by pointer address.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:577: if (type->kind() == Type::kScalar_Kind) {
Why only scalars?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:578: std::vector<std::unique_ptr<Expression>>
parameters;
nit: s/parameters/arguments/ (or just pass {std::move(expr)} below)

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:585: ABORT("cannot coerce %s to %s",
expr->fType->description().c_str(), type->name().c_str());
nit: s/name()/description()/?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:611: }
Don't we need a special case for matrix * matrix also?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:625: // FIXME: need to disallow illegal operations
like vec3 > vec3
Also, bitwise and shift operations only apply to integer scalars and vectors.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:636: if (left->canCoerceTo(right)) {
This case is incorrect for shift operations.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:646: if ((left->columns() > 1) && (right->kind() ==
Type::kScalar_Kind)) {
This case shouldn't apply to arrays, right?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:676: if
(!determine_binary_type(expression.fOperator, left->fType, right->fType,
&leftType,
Seems like we need special handling for logical operators (&&, ||).

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:677: &rightType, &resultType, true)) {
tryFlipped probably shouldn't be true for shift operations.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:704: this->coerce(std::move(left), leftType),
nit: assert not null? (also for next coerce)

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:746: std::vector<std::unique_ptr<Expression>>
parameters) {
nit: s/parameters/arguments/

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:759: parameters[i] =
this->coerce(std::move(parameters[i]), function->fParameters[i]->fType);
1. How does this work for out/inout parameters?
2. Check that coerce returned non-null?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:771: std::vector<std::unique_ptr<Expression>>&
parameters,
nit: s/parameters/arguments/
nit: const?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:791: std::vector<std::unique_ptr<Expression>>
parameters) {
nit: s/parameters/arguments/

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:805: for (auto f : ref->fFunctions) {
nit: const ref

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:813: return this->call(position, best,
std::move(parameters));
nit: std::move(best)

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:831: std::vector<std::unique_ptr<Expression>>
params) {
nit: s/params/args/
nit: odd indentation

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:843: if (type->kind() != Type::kScalar_Kind) {
Is this the correct criteria? I would expect we want to coerce to component type
for matrices, vectors, and arrays, but not for other types, including scalars
and structs.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:844: params[i] = coerce(std::move(params[i]),
type->componentType());
Check that coerce returns non-null?
nit: this->coerce

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:847: actual += params[i]->fType->rows() *
params[i]->fType->columns();
Check that rows() and columns() are not -1?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:850: if ((type->kind() != Type::kVector_Kind ||
actual != 1) &&
nit: Might be a bit clearer to rephrase as:
(actual < min || actual > max) &&
!((type->kind() == Type::kVector_Kind || type->kind() == Type::kMatrix_Kind) &&
(actual == 1))

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:852: (actual < min || actual > max)) {
Checking my understanding: Are all of the following valid?
vec2(vec3(1, 2, 3, 4))
mat2x3(mat3x2(mat2(vec3(1, 2, 3), 4), vec2(5, 6)))
mat2(mat3(vec4(1, 2, 3, 4), vec3(5, 6, 7), vec2(8, 9)))
mat4(0)

(If yes, this language seems insane.)

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:861: return std::unique_ptr<Expression>(new
TernaryExpression(position, std::move(params[0]),
nit: position maybe should be params[0]->fPosition

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:862: std::unique_ptr<Expression>(new
IntLiteral(position, 1)),
nit: odd indentation

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:870: return std::unique_ptr<Expression>(new
Constructor(position, type, std::move(params)));
nit: std::move(type)

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:950: return std::unique_ptr<Expression>(new
IndexExpression(std::move(base), std::move(converted)));
If index is constant and base size is known, should this check that the index is
in bounds?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:954: std::string field) {
nit: const ref?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:961: fErrors.error(base->fPosition, "type '" +
base->fType->description() + "' does not have a "
The spec says something about arrays having a length() method. Are we not
supporting that?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:967: std::string fields) {
nit: const ref?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:968: if (base->fType->columns() == 0) {
nit: Should this just be an assert?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:1028: auto rawParameters = &((ASTCallSuffix&)
*expression.fSuffix).fParameters;
nit: s/parameters/arguments/ in this block

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:1032: *(*rawParameters)[i]);
nit: odd indentation

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:1041: std::string field = ((ASTFieldSuffix&)
*expression.fSuffix).fField;
nit: const ref?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.h:56: IRGenerator(std::shared_ptr<SymbolTable> root,
ErrorReporter& errorReporter);
nit: if the caller will hang on to SymbolTable for the lifetime of IRGenerator,
it's probably better to use a raw pointer (or reference if you prefer) instead
of std::shared_ptr.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.h:58: std::unique_ptr<VarDeclaration>
convertVarDeclaration(ASTVarDeclaration& decl,
nit: document how each method will modify the AST object passed in; or if it
doesn't, make it a const ref. Or maybe pass as std::unique_ptr if the AST object
will not be valid after this call (which would allow the callee to std::move
parts of it).

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.h:71: std::vector<std::unique_ptr<Expression>>
parameters);
nit: s/parameters/arguments/, several places in this file, in the context of
calls.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLParser.cpp
File src/sksl/SkSLParser.cpp (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:292: for (int j = (int) decl->fSizes[i].size() - 1; j >=
0; j--) {
nit: It seems odd that for fields we reverse the array dimensions in the parser
and check that the sizes are constant, but for variables and parameters, we
reverse the array dimensions in the IRGenerator, and check that parameter sizes
are constant.

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

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.h:34: std::shared_ptr<Symbol> operator[](std::string
name) {
nit: const std::string& name

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.h:70: void add(std::string name,
std::shared_ptr<Symbol> symbol) {
nit: const std::string& name

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/sksl.flex
File src/sksl/sksl.flex (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/sksl.flex#new...
src/sksl/sksl.flex:127: "!" { return SkSL::Token::NOT; }
Bitwise not is not supported?

[dogben, 2016-06-23 17:43:12]

Comments on Compiler below. I only have SPIRVCodeGenerator left to review. Save
the best for last, I hope!

Is there anyone else that knows SPIR-V, or do I need to go over the 188-page
spec?

https://codereview.chromium.org/1984363002/diff/380001/gyp/sksl.gyp
File gyp/sksl.gyp (right):

https://codereview.chromium.org/1984363002/diff/380001/gyp/sksl.gyp#newcode1
gyp/sksl.gyp:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved.
This has a different copyright header than other gyp files.

https://codereview.chromium.org/1984363002/diff/380001/gyp/sksl.gypi
File gyp/sksl.gypi (right):

https://codereview.chromium.org/1984363002/diff/380001/gyp/sksl.gypi#newcode1
gyp/sksl.gypi:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved.
This has a different copyright header than other gyp files.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/GLSL.std.450.h
File src/sksl/GLSL.std.450.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/GLSL.std.450....
src/sksl/GLSL.std.450.h:2: ** Copyright (c) 2014-2016 The Khronos Group Inc.
IANAL, but we may need to add a section to LICENSE for this file.
https://skia.googlesource.com/skia/+/master/LICENSE

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLCompiler.cpp
File src/sksl/SkSLCompiler.cpp (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.cpp:46: static auto types =
std::shared_ptr<SymbolTable>(new SymbolTable(*this));
This seems unhygienic. Could we either add a dummy implementation of
ErrorReporter and store it in a static field, or make SymbolTable handle a null
ErrorReporter?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.cpp:47: static auto symbols =
std::shared_ptr<SymbolTable>(new SymbolTable(types, *this));
Why are types and symbols in different SymbolTables?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.cpp:160: (ASTVarDeclaration&) decl,
nit: odd indentation, several places in this method

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.cpp:194: return true;
return fErrorCount == 0? Or make return type void?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.cpp:222: std::string result = fErrorText;
nit: "return fErrorText;" is equivalent.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.cpp:240: cg->generateCode(*program.get(), out);
nit: ".get()" is unnecessary

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.cpp:241: ASSERT(!out.rdstate());
nit: caller's responsibility to check this, and errors writing to a file
shouldn't crash the program.

This would make more sense in the next method, ASSERT(!buffer.rdstate());

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLCompiler.h
File src/sksl/SkSLCompiler.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.h:32: std::unique_ptr<Program>
convertProgram(Program::Kind kind, std::string text);
Will input always be a string, or would it make sense to take the AST as input?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.h:34: bool toSPIRV(Program::Kind kind, std::string text,
std::ostream& out);
nit: wrong indentation

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.h:40: std::string errorText();
nit: const?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.h:47: std::vector<std::unique_ptr<ProgramElement>>*
result);
nit: odd indentation

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.h:50: IRGenerator* fIRGenerator;
std::unique_ptr?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLMain.cpp
File src/sksl/SkSLMain.cpp (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLMain.cpp#...
src/sksl/SkSLMain.cpp:44: if (out.rdstate()) {
nit: out.close() before rdstate

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/spirv.h
File src/sksl/spirv.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/spirv.h#newcode2
src/sksl/spirv.h:2: ** Copyright (c) 2014-2016 The Khronos Group Inc.
IANAL, but we may need to add a section to LICENSE for this file.
https://skia.googlesource.com/skia/+/master/LICENSE

[dogben, 2016-06-24 17:05:29]

Not quite done yet, but I wanted to send my questions and comments about
function and constructor calls.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:447: return std::unique_ptr<FunctionDefinition>(new
FunctionDefinition(f.fPosition, decl,
Check for return at end of body? (Maybe easier to add "bool* endsInReturn" param
to convertBlock.)

The spec doesn't seem to say anything about this, so maybe forgetting the return
is allowed and means the return value is undefined?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:838: int max = type->columns() > 1 ? INT_MAX : min;
This doesn't seem like it would work for struct constructors.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:852: (actual < min || actual > max)) {
On 2016/06/23 at 15:25:10, Ben Wagner wrote:
> Checking my understanding: Are all of the following valid?
> vec2(vec3(1, 2, 3, 4))
> mat2x3(mat3x2(mat2(vec3(1, 2, 3), 4), vec2(5, 6)))
> mat2(mat3(vec4(1, 2, 3, 4), vec3(5, 6, 7), vec2(8, 9)))
> mat4(0)
> 
> (If yes, this language seems insane.)

I happened upon the section of the spec that talks about this. It's insane, but
slightly less insane than I thought. The spec says, "It is a compile-time
error to provide extra arguments beyond this last used argument." My
interpretation is that the following are actually illegal:
vec3(1, 2, 3, 4)
mat2(vec2(1, 2), vec2(3, 4), vec2(5, 6))

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLCompiler.cpp
File src/sksl/SkSLCompiler.cpp (right):

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.cpp:239: auto cg =
std::unique_ptr<SkSL::CodeGenerator>(new SkSL::SPIRVCodeGenerator());
nit: Why not stack-allocate?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:760: parameters[i] =
this->coerce(std::move(parameters[i]), function->fParameters[i]->fType);
The GLSL spec says, "Mismatched types on input parameters (in or inout or
default) must have a conversion from the calling argument type to the formal
parameter type. Mismatched types on output parameters (out or inout) must have a
conversion from the formal parameter type to the calling argument type."

This indicates that coercing the argument to an out parameter to the parameter's
type is not correct. It seems like:
 - for an in/default argument, coerce to parameter type
 - for an out argument whose type doesn't match, but the parameter type can be
coerced to the argument type, we need to create a temporary variable to pass to
the function, then assign the argument to the temporary variable.
 - for inout arguments, the types must match exactly (I think?).

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:838: int min = type->rows() * type->columns();
I found the following in the GLSL spec: "If a matrix is constructed from a
matrix, then each component (column i, row j) in the result that has a
corresponding component (column i, row j) in the argument will be initialized
from there. All other components will be initialized to the identity matrix. If
a matrix argument is given to a matrix constructor, it is a compile-time error
to have any other arguments."

I.e. both "mat3(mat2(...))" and "mat2(mat3(...))" are valid.

Maybe we should just limit SkSL to the "sane" subset of vector and matrix
constructors?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:844: if (type->kind() != Type::kScalar_Kind) {
nit: probably can do this for scalars also. There is a check below for "if
(params.size() == 1 && params[0]->fType == type)" that would ensure no no-op
constructors.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:848: actual += params[i]->fType->rows() *
params[i]->fType->columns();
IMO, somewhere in this function, you should re-arrange vector/matrix arguments
to fit into the result type. This would simplify the SPIR-V CodeGenerator and
any future CodeGenerators. However, it means you would need to introduce
temporary variables. E.g.:

- "mat2(vec3(a, b, c), d)" would need to be translated into:
vec3 tmp1 = vec3(a, b, c);
mat2(tmp1.xy, vec2(tmp1.z, d));

- "mat2(mat3(a + b))" would need to be translated into:
float tmp2 = a + b;
mat3 tmp3 = mat3(vec3(tmp2, 0, 0), vec3(0, tmp2, 0), vec3(0, 0, tmp2));
mat2(tmp3[0].xy, tmp3[1].xy)

- "mat3(mat2(a))" would need to be translated into:
mat2 tmp4 = mat2(vec2(a, 0), vec2(0, a));
mat3(vec3(tmp4[0].x, tmp4[0].y, 0), vec3(tmp4[1].x, tmp4[1].y, 1))

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:863: std::unique_ptr<Expression>(new
IntLiteral(position, 1)),
type->isNumber() is true for floats, but this will return an integer expression.

(Not sure if you want to change this to type == integer, or if you want to add
cases for int vs. float. The GLSL spec says there's a "float(bool)" constructor,
but I would be ok with SkSL only allowing bool to int conversions.)

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:865: }
Seems that you should check that the argument is a number if it's not a bool.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:871: return std::unique_ptr<Expression>(new
Constructor(position, type, std::move(params)));
Do we need to check the argument types for struct constructors?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
File src/sksl/SkSLSPIRVCodeGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:36: fIntrinsicMap["round"]         =
ALL_GLSL(Round);
nit: In a future CL, maybe this could be a one-time initialization using Symbol
as the key? I don't know how much that would improve speed.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:176: static std::string opcode_text(SpvOp_
opCode) {
nit: Presumably this was generated from the definition of SpvOp_. In a future
CL, could we make this a generated source file? Or move to a different file to
make this file 600 lines shorter?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:784: case SpvOpConstant:          // fall
through
nit: odd indentation

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:835: this->writeWord((2 << 16) | SpvOpLabel,
out);
nit: should this be writeOpCode(SpvOpLabel, 2, out) or
writeInstruction(SpvOpLabel, label, out)?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:978: void
SPIRVCodeGenerator::writeStruct(std::shared_ptr<Type> type, SpvId resultId) {
nit: This method doesn't take a ref on type, so it should accept Type* rather
than shared_ptr<Type>. Same for many other methods in this file.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:981: for (auto f : type->fields()) {
nit: const ref

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:982:
types.push_back(this->getType(f.fType));
nit: Since this method is mutually recursive with getType, it seems less
error-prone to write to temporary name/constant/decoration buffers and then
write the temporary buffers to the result at the end of the method.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1001:
this->writeInstruction(SpvOpMemberDecorate, resultId, (SpvId) i,
SpvDecorationOffset,
nit: I don't understand why this is cast to SpvId.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1014: offset += alignment - offset %
alignment;
ISTM that this shouldn't happen unless Type::size() is returning the incorrect
value.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1028: }
nit: inconsistent placement for else in this file

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1064: (SpvId) type->stride(),
fDecorationBuffer);
nit: cast to int32_t?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1132: //            if
(is_out(function->fParameters[i])) {
Please xref this comment with a comment in writeFunctionCall.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1165: SpvId
SPIRVCodeGenerator::writeExpression(Expression& expr, std::ostream& out) {
nit: (Maybe better to think about this for a future CL:) It might be useful for
writeExpression to take the result SpvId as an argument instead of returning it.
This would allow:
- if the result of the expression is unused, no ID is consumed (not sure how
closely IDs correspond to registers in SPIR-V).
- if the result of the expression will be stored, the result can be written
directly to the storage location rather than going to a temp register (again,
not sure if this is optimized away in SPIR-V-to-native compiler).
- for expressions with no side-effects, if the result is unused, the expression
can be omitted entirely. (Well, this might be a bit tricky, and is probably
better suited to a different compiler stage.)

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1201: auto intrinsic =
fIntrinsicMap.find(c.fFunction->fName);
nit: const ref

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1222:
arguments.push_back(this->writeExpression(*c.fArguments[i], out));
Intrinsics do not have any out/inout parameters?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1248: return this->writeSpecialIntrinsic(c,
(SpecialIntrinsic) intrinsicId, out);
nit: The SpvId stored in result is unused.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1275: SpvId sampler =
this->writeExpression(*c.fArguments[0], out);
nit: wrong indentation

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1308:
this->writeExpression(*c.fArguments[0], out),
Order of evaluation of arguments is unspecified in C++.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1349: SpvId
SPIRVCodeGenerator::writeConstructor(Constructor& c, std::ostream& out) {
nit: maybe split this method up?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1357: if (c.fArguments.size() == 1) {
Seems like this doesn't correctly handle "vec3(vec4(1, 2, 3, 4))". I believe
isConstant will return true for such an expression, but it's not correct to
repeat the first argument 3 times.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1358:
this->writeOpCode(SpvOpConstantComposite, 3 + c.fType->columns(),
fConstantBuffer);
nit: Is this not applicable to matrices, and/or does a matrix equivalent not
exist?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1364: } else {
Does SpvOpConstantComposite expect scalars as arguments? If so, this doesn't
correctly handle "vec3(vec2(1, 2), 3)".

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1379: if (c.fArguments[0]->fType ==
kInt_Type) {
Missing case for kDouble_Type?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1385: } else if (c.fArguments[0]->fType ==
kFloat_Type) {
nit: Seems like this case is eliminated by IRGenerator.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1390: else if (c.fType == kInt_Type) {
nit: inconsistent formatting of "else if" in this file.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1394: if (c.fArguments[0]->fType ==
kFloat_Type) {
Missing case for kDouble_Type?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1400: } else if (c.fArguments[0]->fType ==
kInt_Type) {
nit: Seems like this case is eliminated by IRGenerator.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1405: ASSERT(!c.fType->isNumber());
There are cases missing for double and uint.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1408:
arguments.push_back(this->writeExpression(*c.fArguments[i], out));
nit: Very minor, probably not worth worrying about, but if the SPIR-V-to-native
compiler doesn't do reordering, then evaluating all arguments before
constructing matrix columns will use more registers than necessary.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1412: if (arguments.size() == 1) {
nit: maybe add a short comment explaining what a single-arg vector/matrix
constructor means.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1413: if (c.fType->kind() ==
Type::kVector_Kind) {
Similar question as above: Does this work for e.g. "vec2(my_vec3)"?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1421: ASSERT(c.fType->kind() ==
Type::kMatrix_Kind);
Where are structs and arrays handled?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1427:
this->writeOpCode(SpvOpCompositeConstruct, 3 + c.fType->rows(),
Missing result id. ("3 + c.fType->rows()" doesn't match the number of words
written.)

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1437: for (SpvId id : columnIds) {
columnIds is not populated.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1440: this->writeWord(result, out);
Wrong order for result word?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1447: ASSERT(currentCount == 0);
Wouldn't this assert trigger for e.g. "mat2(a, vec3(b, c, d))"?

However, see comment in IRGenerator::convertConstructor.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1449: currentCount = 0;
Isn't this incorrect for e.g. "mat2(vec3(a, b, c), d)"?

However, see comment in IRGenerator::convertConstructor.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1472:
this->writeOpCode(SpvOpCompositeConstruct, 3 + (int32_t) c.fArguments.size(),
out);
Is this case for arrays?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1543: SpvId result = this->nextId();
This doesn't seem right. If we're asking for an lvalue, most likely there's an
lvalue at the source level, so creating an anonymous temporary means that the
actual lvalue is not being assigned.

One reason this might come up is due to the out param issue I commented on in
IRGenerator.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
File src/sksl/SkSLSPIRVCodeGenerator.h (right):

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.h:212: typedef std::tuple<IntrinsicKind,
int32_t, int32_t, int32_t, int32_t> Intrinsic;
nit: define a struct?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.h:214:
std::unordered_map<std::shared_ptr<FunctionDeclaration>, SpvId> fFunctionMap;
nit: Seems like fFunctionMap, fVariableMap, and fInterfaceBlockMap should use
raw pointer rather than shared_ptr.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.h:217: std::unordered_map<std::string, SpvId>
fTypeMap;
nit: Seems like it would be better to have fTypeMap keyed by Type*,
fFunctionTypeMap keyed by FuctionDeclaration*, and fPointerTypeMap keyed by
pair<Type*,SpvStorageClass_>. Hashing pointers is much faster than hashing
strings.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/ir/SkSLIndexE...
File src/sksl/ir/SkSLIndexExpression.h (right):

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/ir/SkSLIndexE...
src/sksl/ir/SkSLIndexExpression.h:19: static std::shared_ptr<Type>
index_type(std::shared_ptr<Type> type) {
nit: maybe move this to a static method IndexExpression::ResultType?

nit: Skia style says the name should be IndexType.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/ir/SkSLSwizzle.h
File src/sksl/ir/SkSLSwizzle.h (right):

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/ir/SkSLSwizzl...
src/sksl/ir/SkSLSwizzle.h:21: static std::shared_ptr<Type> get_type(Expression&
value,
nit: Skia style says this should be named GetType.

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

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:128: , fComponentType(std::move(componentType))
nit: Maybe assert that componentType is a scalar?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:156: , fComponentType(std::move(componentType))
nit: Maybe assert that componentType is a scalar?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:404: const SpvDim_ fDimensions;
Is this a TODO? It seems that everything in the IR is (more-or-less)
target-agnostic except this field.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/sksl.flex
File src/sksl/sksl.flex (right):

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/sksl.flex#new...
src/sksl/sksl.flex:31: {DIGIT}+ { return SkSL::Token::INT_LITERAL; }
Spec mentions octal and hexadecimal numbers. Is that not supported? Maybe we
should error for numbers beginning with 0 to preserve compatibility?

[ethannicholas, 2016-06-24 21:23:12]

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

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.c...
src/sksl/ir/SkSLType.cpp:131: ABORT("unsupported scalar_to_compound type %s",
this->description().c_str());
On 2016/06/21 21:52:51, Ben Wagner wrote:
> bool not needed?

It'll come eventually. Support for int and bool compound types is a bit
incomplete at the moment simply because virtually no shaders use them (and none
at all in Skia).

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.c...
src/sksl/ir/SkSLType.cpp:239: const std::shared_ptr<Type> kGSampler1D_Type(new
Type("$gsampler1D", type(kSampler1D_Type)));
On 2016/06/21 21:52:51, Ben Wagner wrote:
> I don't understand this. We're passing the result of type to fCoercibleTypes.
> type gives me the same type 4 times. Why would we want the same type 4 times
on
> the coercion list?

Added the missing FIXME comment. Basically, the gsampler types are (as the g
implies) generic... but I haven't managed to figure out exactly how they're
supposed to behave. So for right now I'm just mapping them onto a single type
and that's working fine.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.c...
src/sksl/ir/SkSLType.cpp:275: const std::shared_ptr<Type> kVec_Type(new
Type("$vec", types(kVec2_Type, kVec2_Type, kVec3_Type,
On 2016/06/21 21:52:51, Ben Wagner wrote:
> kVec2_Type twice is correct?

Was written before kInvalid_Type existed and I forgot to update it. Fixed.

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

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:246: size_t alignment() {
On 2016/06/21 21:52:52, Ben Wagner wrote:
> I don't understand a few of these calculations.

That's because they're stupid, but based on the spec. 7.6.2.2: Standard Uniform
Block Layout. I added a comment to make that clear.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:250: case kVector_Kind: // fall through
On 2016/06/21 21:52:51, Ben Wagner wrote:
> no fall through

Well... there USED to be :-)

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:263: ABORT("unsupported array type");
On 2016/06/21 21:52:51, Ben Wagner wrote:
> What about multi-dimensional arrays?

Haven't done the slightest bit of testing around them yet, figured hitting an
assertion failure was fine for the time being. I've never encountered a shader
which uses them.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:293: return 16;
On 2016/06/21 21:52:51, Ben Wagner wrote:
> Is this correct for kDMat4x4_Type? Seems like it should be 32 (4 components *
8
> bytes per component).

For the time being, we only support 32 bit numbers. There are a few comments
here and there talking about that, I'll add another one.

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:295: return 16;
On 2016/06/21 21:52:51, Ben Wagner wrote:
> Shouldn't this be based on the component size and align?

I spent days trying to figure out the insane OpenGL alignment rules, and forgot
to un-hack this once I got it working :-)

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:315: return fColumns * this->alignment();
On 2016/06/21 21:52:51, Ben Wagner wrote:
> Shouldn't this be fColumns * (fComponentType->size() rounded up to the next
> fComponentType->alignment())?

You'd think so, but no. An OpenGL float[] is 16-byte aligned, and each
individual float consumes 16 bytes.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLCompiler.cpp
File src/sksl/SkSLCompiler.cpp (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.cpp:46: static auto types =
std::shared_ptr<SymbolTable>(new SymbolTable(*this));
On 2016/06/23 17:43:11, Ben Wagner wrote:
> This seems unhygienic. Could we either add a dummy implementation of
> ErrorReporter and store it in a static field, or make SymbolTable handle a
null
> ErrorReporter?

These didn't used to be shared, and I failed to fix this when I made them
static. Fixed.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.cpp:47: static auto symbols =
std::shared_ptr<SymbolTable>(new SymbolTable(types, *this));
On 2016/06/23 17:43:12, Ben Wagner wrote:
> Why are types and symbols in different SymbolTables?

The Parser uses the types symboltable to determine whether an IDENTIFIER is a
type. I've added a comment to clarify that.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:110: default:
On 2016/06/23 15:25:08, Ben Wagner wrote:
> nit: Do we compile with the warning enabled when not all values of the enum
are
> handled?

Without the 'default:' it works on most platforms, but the Android compiler
complains about the function not always returning.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:138: return Modifiers(modifiers);
On 2016/06/23 15:25:08, Ben Wagner wrote:
> Shouldn't we check that modifiers don't conflict?

It's on my list. This is just the initial checkin; it wasn't my intent to have
everything foam-padded for safety at this point :-).

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:153: std::vector<std::unique_ptr<Expression>>
currentVarSizes;
On 2016/06/23 15:25:08, Ben Wagner wrote:
> If baseType is an array type, should the sizes be added to currentVarSizes
> before iterating over fSizes?

baseType can't be an array type, since it's the sizes that make array types. But
I'll add an assert just to make that clear.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:167: count = -1;
On 2016/06/23 15:25:10, Ben Wagner wrote:
> Check that size's type is integer?

Done.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:254: std::unique_ptr<Expression> test =
this->convertExpression(*d.fTest);
On 2016/06/23 15:25:10, Ben Wagner wrote:
> Other tests are coerced. Why not this one?

Oversight. Fixed.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:276: return std::unique_ptr<Statement>(new
ExpressionStatement(std::move(e)));
On 2016/06/23 15:25:11, Ben Wagner wrote:
> Check that e is not FunctionReference or TypeReference?

Done.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:316: static std::shared_ptr<Type>
expand_generics(std::shared_ptr<Type> type, int i) {
On 2016/06/23 15:25:09, Ben Wagner wrote:
> nit: This function should probably accept and return Type* rather than
> shared_ptr<Type>.

I don't think it can. If it returns Type*, then I would have to wrap it in
std::shared_ptr<Type> to use it where it needs to be passed, and then I'd have
an ownership conflict. And since it needs to be able to return its parameter, it
has to take a shared_ptr also.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:419: fSymbolTable->add(parameters[i]->fName,
decl->fParameters[i]);
On 2016/06/23 15:25:11, Ben Wagner wrote:
> Are modifiers allowed to differ between declaration and definition (like const
> in C++)?

Good catch. No, they should not, I will add an error check for that.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:442: std::unique_ptr<Block> body =
this->convertBlock(*f.fBody);
On 2016/06/23 15:25:09, Ben Wagner wrote:
> Seems like a very minor issue, but the spec says "A function's parameter
> declarations and body together form a single scope nested in the global
scope,"
> and "...a declared name cannot be redeclared in the same scope..."
> 
> convertBlock uses AutoSymbolTable to start a new scope, so the function body
is
> in a different scope from the parameters, and therefore a local var could
shadow
> a parameter.

I might end up changing this, but in general it is not my intent to slavishly
adhere to the GLSL spec. This is not a GLSL compiler, and the language will
become more different as we proceed.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:447: return std::unique_ptr<FunctionDefinition>(new
FunctionDefinition(f.fPosition, decl,
On 2016/06/24 17:05:26, Ben Wagner wrote:
> Check for return at end of body? (Maybe easier to add "bool* endsInReturn"
param
> to convertBlock.)
> 
> The spec doesn't seem to say anything about this, so maybe forgetting the
return
> is allowed and means the return value is undefined?

You need control flow analysis to prove that a function returns, and I don't
perform that analysis yet. It's coming.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:463:
fields.push_back(Type::Field(decl->fVars[j]->fModifiers, decl->fVars[j]->fName,
On 2016/06/23 15:25:08, Ben Wagner wrote:
> Shouldn't we OR in mods.fFlags? And check that they don't conflict?

There are probably some illegal combinations we should reject -- I'll look into
that -- but no, we shouldn't be ORing them together. These are members of a
struct, not top-level variables in their own right, and the top-level
declarations go on the struct in the output, rather than on the individual
fields (OpDecorate vs. OpMemberDecorate).

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:577: if (type->kind() == Type::kScalar_Kind) {
On 2016/06/23 15:25:09, Ben Wagner wrote:
> Why only scalars?

Because we don't currently support compound types that contain anything other
than float, so only scalars actually get coerced.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:578: std::vector<std::unique_ptr<Expression>>
parameters;
On 2016/06/23 15:25:11, Ben Wagner wrote:
> nit: s/parameters/arguments/ (or just pass {std::move(expr)} below)

This would have been easier had I realized you could create vectors with that
syntax before you told me :-)

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:611: }
On 2016/06/23 15:25:09, Ben Wagner wrote:
> Don't we need a special case for matrix * matrix also?

Non-square matrices are on my list; obviously there's a lot of the groundwork
laid, but they don't actually work (and we don't use them).

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:625: // FIXME: need to disallow illegal operations
like vec3 > vec3
On 2016/06/23 15:25:11, Ben Wagner wrote:
> Also, bitwise and shift operations only apply to integer scalars and vectors.

Only floats are fully supported for now, I've added a comment to that effect.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:646: if ((left->columns() > 1) && (right->kind() ==
Type::kScalar_Kind)) {
On 2016/06/23 15:25:11, Ben Wagner wrote:
> This case shouldn't apply to arrays, right?

Good catch. This was written long before I added support for arrays.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:676: if
(!determine_binary_type(expression.fOperator, left->fType, right->fType,
&leftType,
On 2016/06/23 15:25:10, Ben Wagner wrote:
> Seems like we need special handling for logical operators (&&, ||).

In what way?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:677: &rightType, &resultType, true)) {
On 2016/06/23 15:25:11, Ben Wagner wrote:
> tryFlipped probably shouldn't be true for shift operations.

It doesn't actually flip the operation around, it just determines the types as
if it had (so I don't have to duplicate the block of code that checks left
coercing to right and matrix * scalar).

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:759: parameters[i] =
this->coerce(std::move(parameters[i]), function->fParameters[i]->fType);
On 2016/06/23 15:25:09, Ben Wagner wrote:
> 1. How does this work for out/inout parameters?

Good catch, was missing a markWrittenTo here.

> 2. Check that coerce returned non-null?

Don't need an early return for a null coercion, as it will have reported an
error but that doesn't stop us from evaluating the rest of the parameters.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:838: int max = type->columns() > 1 ? INT_MAX : min;
On 2016/06/24 17:05:26, Ben Wagner wrote:
> This doesn't seem like it would work for struct constructors.

We don't use them. I've got it on my list of things to add support for.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:843: if (type->kind() != Type::kScalar_Kind) {
On 2016/06/23 15:25:10, Ben Wagner wrote:
> Is this the correct criteria? I would expect we want to coerce to component
type
> for matrices, vectors, and arrays, but not for other types, including scalars
> and structs.

I only officially support float vectors and matrices at the moment (and this is
all Skia uses), so there's never actually any coercion for them. Obviously I've
laid a lot of the groundwork for other types, but I'm not worried about having
it all perfect in the first release.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:844: params[i] = coerce(std::move(params[i]),
type->componentType());
On 2016/06/23 15:25:11, Ben Wagner wrote:
> Check that coerce returns non-null?
> nit: this->coerce

Fixed the nit, but I don't think the null check matters -- if the result is
null, an error will already have been reported, so we might as well continue
checking the other parameters.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:847: actual += params[i]->fType->rows() *
params[i]->fType->columns();
On 2016/06/23 15:25:11, Ben Wagner wrote:
> Check that rows() and columns() are not -1?

rows() and columns() will both generate an assertion error if the count is
invalid (which it will only be if someone writes some really weird code). While
this is not ideal, this is just the initial checkin of the compiler and I'm
aware of several ways to write code sufficiently insane that it causes an
assertion failure. I'll be tightening things up over time.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:852: (actual < min || actual > max)) {
On 2016/06/23 15:25:10, Ben Wagner wrote:
> Checking my understanding: Are all of the following valid?
> vec2(vec3(1, 2, 3, 4))

Yes! This is permitted by GLSL, though I am considering removing this "feature"
in SkSL (unfortunately, Skia actually does create a vector with too many
parameters in at least one spot).

> mat2x3(mat3x2(mat2(vec3(1, 2, 3), 4), vec2(5, 6)))
> mat2(mat3(vec4(1, 2, 3, 4), vec3(5, 6, 7), vec2(8, 9)))

These are currently accepted but shouldn't be. It's on my list to tighten this
up.

> mat4(0)

Yes! This results in a matrix in which all components are zero. mat4(1) would
give you a standard 4x4 identity matrix.

> (If yes, this language seems insane.)

Yes!

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:950: return std::unique_ptr<Expression>(new
IndexExpression(std::move(base), std::move(converted)));
On 2016/06/23 15:25:09, Ben Wagner wrote:
> If index is constant and base size is known, should this check that the index
is
> in bounds?

I was planning to worry about that when I've got a dataflow analyzer in place.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:961: fErrors.error(base->fPosition, "type '" +
base->fType->description() + "' does not have a "
On 2016/06/23 15:25:11, Ben Wagner wrote:
> The spec says something about arrays having a length() method. Are we not
> supporting that?

Not yet, at least.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLParser.cpp
File src/sksl/SkSLParser.cpp (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:292: for (int j = (int) decl->fSizes[i].size() - 1; j >=
0; j--) {
On 2016/06/23 15:25:12, Ben Wagner wrote:
> nit: It seems odd that for fields we reverse the array dimensions in the
parser
> and check that the sizes are constant, but for variables and parameters, we
> reverse the array dimensions in the IRGenerator, and check that parameter
sizes
> are constant.

I agree that it's not ideal, but did not feel it was worth the refactoring
effort at the time to fix it. It just worked out that (without major refactoring
effort) I had to know the array sizes at this point in order to create the Type,
whereas the other cases do not require the Type to be created so early. I will
definitely reexamine this in the future, but don't think it's worth it right
this second.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLBlock.h
File src/sksl/ir/SkSLBlock.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLBlock....
src/sksl/ir/SkSLBlock.h:29: result += "\n}\n";
On 2016/06/22 17:43:56, Ben Wagner wrote:
> nit: final newline unnecessary?

No, the newline is there so the code generates like:

{
foo();
}
nextStatement();

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLBreakS...
File src/sksl/ir/SkSLBreakStatement.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLBreakS...
src/sksl/ir/SkSLBreakStatement.h:19: struct BreakStatement : public Statement {
On 2016/06/22 17:43:56, Ben Wagner wrote:
> In my experience, breaks and continues disappear when converting AST to IR.
But
> maybe it's useful to keep around if you are targeting GLSL.

Breaks and continues would normally turn into gotos, but as you indicate I need
to be able to target a language that does not have goto.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLConstr...
File src/sksl/ir/SkSLConstructor.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLConstr...
src/sksl/ir/SkSLConstructor.h:20: std::vector<std::unique_ptr<Expression>>
parameters)
On 2016/06/22 17:43:57, Ben Wagner wrote:
> nit: s/parameters/arguments/g

Wow. I had always thought those were interchangeable synonyms. TIL.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLDoStat...
File src/sksl/ir/SkSLDoStatement.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLDoStat...
src/sksl/ir/SkSLDoStatement.h:19: struct DoStatement : public Statement {
On 2016/06/22 17:43:57, Ben Wagner wrote:
> Similar to comment above, in my experience, loops disappear in IR.

Again, that's only an option when you have access to goto, which I don't :-(.
Furthermore, even SPIR-V (which does include a goto) requires that you know
you're dealing with a loop because of its (mandatory) structured loop ops.

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

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLForSta...
src/sksl/ir/SkSLForStatement.h:34: result += " ";
On 2016/06/22 17:43:57, Ben Wagner wrote:
> ;

No, the initializer is a statement and thus already ends in ';'

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLFuncti...
File src/sksl/ir/SkSLFunctionCall.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLFuncti...
src/sksl/ir/SkSLFunctionCall.h:20: FunctionCall(Position position,
std::shared_ptr<FunctionDeclaration> function,
On 2016/06/22 17:43:57, Ben Wagner wrote:
> Checking my understanding: Reading the GLSL spec, it sounds like functions can
> only be called by name. Even the "subroutine" thing looks like a function
> declaration. So, things like "(b ? f1 : f2)(arg1, arg2)" are not allowed.
> Correct?

Correct.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLFuncti...
File src/sksl/ir/SkSLFunctionDeclaration.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLFuncti...
src/sksl/ir/SkSLFunctionDeclaration.h:45: mutable bool fDefined;
On 2016/06/22 17:43:57, Ben Wagner wrote:
> nit: seems like this doesn't need to be mutable.

While you are correct that it isn't needed from a "modifiable via a const
function" standpoint, I wanted to call out that this field (unlike every other
field across all of the IR) violates immutability. Might be silly, but I'm used
to working in languages with first-class immutability and wanted to be explicit
about it...

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLLayout.h
File src/sksl/ir/SkSLLayout.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLLayout...
src/sksl/ir/SkSLLayout.h:57: result = "layout (" + result + ")";
On 2016/06/22 17:43:57, Ben Wagner wrote:
> nit: just return?

It's a stylistic point for me -- I'm certainly willing to do early returns if it
simplifies the flow, but if it doesn't make things more readable or performant
I'd prefer to just let the return that's only one statement farther down handle
it.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLSwizzle.h
File src/sksl/ir/SkSLSwizzle.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLSwizzl...
src/sksl/ir/SkSLSwizzle.h:21: static std::shared_ptr<Type> get_type(Expression&
value,
On 2016/06/22 17:43:57, Ben Wagner wrote:
> nit: I'm used to all references being const, but I don't know what style Skia
> normally uses.

Normally I would have used const, but all of these types are immutable in the
first place -- does it matter?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLSwizzl...
src/sksl/ir/SkSLSwizzle.h:29: case 1: return kFloat_Type;
On 2016/06/22 17:43:57, Ben Wagner wrote:
> nit: Aren't all the "case 1"s in this function redundant?

What do you mean?

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

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:32: std::string description() {
On 2016/06/22 17:43:58, Ben Wagner wrote:
> override? const?

You are correct that it should have been const, but it's not overriding
anything.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:217: int dimensions() {
On 2016/06/22 17:43:58, Ben Wagner wrote:
> nit: should this and all following methods be const?

The fields should all have been defined const (which I have fixed...). With the
entire class being immutable, I don't generally worry about putting const on the
methods -- do you think it matters?

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLVarDec...
File src/sksl/ir/SkSLVarDeclaration.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLVarDec...
src/sksl/ir/SkSLVarDeclaration.h:18: * A variable declaration, which may consist
of multiple individual variables. For instance
On 2016/06/22 17:43:58, Ben Wagner wrote:
> Is there any reason not to split the multiple variable case into separate
> individual VarDeclaration objects for IR? Seems like that would make things
> simpler.

I had started to do it that way, but then a single variable declaration would
turn into multiple statements, which complicated handling in various other
spots. Ultimately this seemed less painful to me.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/sksl.flex
File src/sksl/sksl.flex (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/sksl.flex#new...
src/sksl/sksl.flex:127: "!" { return SkSL::Token::NOT; }
On 2016/06/23 15:25:12, Ben Wagner wrote:
> Bitwise not is not supported?

Not currently, no. Bitwise operations are not even supported on most hardware,
and certainly none of our shaders use them.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLCompiler.cpp
File src/sksl/SkSLCompiler.cpp (right):

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.cpp:239: auto cg =
std::unique_ptr<SkSL::CodeGenerator>(new SkSL::SPIRVCodeGenerator());
On 2016/06/24 17:05:26, Ben Wagner wrote:
> nit: Why not stack-allocate?

I don't remember why I was originally allocating it, but you are of course
correct that it is no longer necessary.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:760: parameters[i] =
this->coerce(std::move(parameters[i]), function->fParameters[i]->fType);
On 2016/06/24 17:05:26, Ben Wagner wrote:
> The GLSL spec says, "Mismatched types on input parameters (in or inout or
> default) must have a conversion from the calling argument type to the formal
> parameter type. Mismatched types on output parameters (out or inout) must have
a
> conversion from the formal parameter type to the calling argument type."
> 
> This indicates that coercing the argument to an out parameter to the
parameter's
> type is not correct. It seems like:
>  - for an in/default argument, coerce to parameter type
>  - for an out argument whose type doesn't match, but the parameter type can be
> coerced to the argument type, we need to create a temporary variable to pass
to
> the function, then assign the argument to the temporary variable.
>  - for inout arguments, the types must match exactly (I think?).

Again, not stressed about following the GLSL spec exactly, but I will add a note
to handle this situation properly just as soon as I decide what "properly"
means.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:838: int min = type->rows() * type->columns();
On 2016/06/24 17:05:26, Ben Wagner wrote:
> I found the following in the GLSL spec: "If a matrix is constructed from a
> matrix, then each component (column i, row j) in the result that has a
> corresponding component (column i, row j) in the argument will be initialized
> from there. All other components will be initialized to the identity matrix.
If
> a matrix argument is given to a matrix constructor, it is a compile-time error
> to have any other arguments."
> 
> I.e. both "mat3(mat2(...))" and "mat2(mat3(...))" are valid.
> 
> Maybe we should just limit SkSL to the "sane" subset of vector and matrix
> constructors?

I'm definitely unsure that we want to copy GLSL here.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:844: if (type->kind() != Type::kScalar_Kind) {
On 2016/06/24 17:05:27, Ben Wagner wrote:
> nit: probably can do this for scalars also. There is a check below for "if
> (params.size() == 1 && params[0]->fType == type)" that would ensure no no-op
> constructors.

Without this check, we will run into infinite recursion -- we have float(x), so
we coerce x to float, which means we have float(x)...

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:848: actual += params[i]->fType->rows() *
params[i]->fType->columns();
On 2016/06/24 17:05:26, Ben Wagner wrote:
> IMO, somewhere in this function, you should re-arrange vector/matrix arguments
> to fit into the result type. This would simplify the SPIR-V CodeGenerator and
> any future CodeGenerators. However, it means you would need to introduce
> temporary variables. E.g.:
> 
> - "mat2(vec3(a, b, c), d)" would need to be translated into:
> vec3 tmp1 = vec3(a, b, c);
> mat2(tmp1.xy, vec2(tmp1.z, d));
> 
> - "mat2(mat3(a + b))" would need to be translated into:
> float tmp2 = a + b;
> mat3 tmp3 = mat3(vec3(tmp2, 0, 0), vec3(0, tmp2, 0), vec3(0, 0, tmp2));
> mat2(tmp3[0].xy, tmp3[1].xy)
> 
> - "mat3(mat2(a))" would need to be translated into:
> mat2 tmp4 = mat2(vec2(a, 0), vec2(0, a));
> mat3(vec3(tmp4[0].x, tmp4[0].y, 0), vec3(tmp4[1].x, tmp4[1].y, 1))

It's not my intent to leave all of those combinations legal. I will definitely
be tightening this up down the road.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:865: }
On 2016/06/24 17:05:27, Ben Wagner wrote:
> Seems that you should check that the argument is a number if it's not a bool.

Why? If we're doing a number->number cast, it just ends up creating a
Constructor below, which is what we want.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
File src/sksl/SkSLSPIRVCodeGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:36: fIntrinsicMap["round"]         =
ALL_GLSL(Round);
On 2016/06/24 17:05:27, Ben Wagner wrote:
> nit: In a future CL, maybe this could be a one-time initialization using
Symbol
> as the key? I don't know how much that would improve speed.

It's on my list :-)

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:784: case SpvOpConstant:          // fall
through
On 2016/06/24 17:05:28, Ben Wagner wrote:
> nit: odd indentation

I need to figure out which one of my editors snuck these stupid tabs in...

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:835: this->writeWord((2 << 16) | SpvOpLabel,
out);
On 2016/06/24 17:05:28, Ben Wagner wrote:
> nit: should this be writeOpCode(SpvOpLabel, 2, out) or
> writeInstruction(SpvOpLabel, label, out)?

I don't remember why it had to be different originally, but you're certainly
right that it shouldn't be anymore.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1014: offset += alignment - offset %
alignment;
On 2016/06/24 17:05:28, Ben Wagner wrote:
> ISTM that this shouldn't happen unless Type::size() is returning the incorrect
> value.

Normal, sane languages pad the *beginning* of the value if needed to bring it to
an even multiple of alignment. OpenGL does that, but also requires padding at
the *end* if size is less than alignment.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1028: }
On 2016/06/24 17:05:28, Ben Wagner wrote:
> nit: inconsistent placement for else in this file

Ack. 30 year habits are hard to break :-(

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1165: SpvId
SPIRVCodeGenerator::writeExpression(Expression& expr, std::ostream& out) {
On 2016/06/24 17:05:28, Ben Wagner wrote:
> nit: (Maybe better to think about this for a future CL:) It might be useful
for
> writeExpression to take the result SpvId as an argument instead of returning
it.
> This would allow:
> - if the result of the expression is unused, no ID is consumed (not sure how
> closely IDs correspond to registers in SPIR-V).
> - if the result of the expression will be stored, the result can be written
> directly to the storage location rather than going to a temp register (again,
> not sure if this is optimized away in SPIR-V-to-native compiler).
> - for expressions with no side-effects, if the result is unused, the
expression
> can be omitted entirely. (Well, this might be a bit tricky, and is probably
> better suited to a different compiler stage.)

An expression might not result in a new SpvId being allocated (e.g. ten
references to the constant 1 will result in only one OpConstant being written),
so it's really up to the expression to know whether it needs a new id or not.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1222:
arguments.push_back(this->writeExpression(*c.fArguments[i], out));
On 2016/06/24 17:05:28, Ben Wagner wrote:
> Intrinsics do not have any out/inout parameters?

None of the ones we currently support.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1248: return this->writeSpecialIntrinsic(c,
(SpecialIntrinsic) intrinsicId, out);
On 2016/06/24 17:05:27, Ben Wagner wrote:
> nit: The SpvId stored in result is unused.

Fixed.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1308:
this->writeExpression(*c.fArguments[0], out),
On 2016/06/24 17:05:28, Ben Wagner wrote:
> Order of evaluation of arguments is unspecified in C++.
Fixed.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1349: SpvId
SPIRVCodeGenerator::writeConstructor(Constructor& c, std::ostream& out) {
On 2016/06/24 17:05:28, Ben Wagner wrote:
> nit: maybe split this method up?

It was so short and simple when I first wrote it :-). Yes, past time to refactor
it.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1357: if (c.fArguments.size() == 1) {
On 2016/06/24 17:05:27, Ben Wagner wrote:
> Seems like this doesn't correctly handle "vec3(vec4(1, 2, 3, 4))". I believe
> isConstant will return true for such an expression, but it's not correct to
> repeat the first argument 3 times.

Good catch; it was working despite this error.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1358:
this->writeOpCode(SpvOpConstantComposite, 3 + c.fType->columns(),
fConstantBuffer);
On 2016/06/24 17:05:28, Ben Wagner wrote:
> nit: Is this not applicable to matrices, and/or does a matrix equivalent not
> exist?

The matrix equivalent is much more complicated because I have to create
temporary vectors.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1405: ASSERT(!c.fType->isNumber());
On 2016/06/24 17:05:27, Ben Wagner wrote:
> There are cases missing for double and uint.

Unsupported for the time being.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1421: ASSERT(c.fType->kind() ==
Type::kMatrix_Kind);
On 2016/06/24 17:05:27, Ben Wagner wrote:
> Where are structs and arrays handled?

They aren't yet.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1427:
this->writeOpCode(SpvOpCompositeConstruct, 3 + c.fType->rows(),
On 2016/06/24 17:05:27, Ben Wagner wrote:
> Missing result id. ("3 + c.fType->rows()" doesn't match the number of words
> written.)

Yeah, this was all screwy -- a logic error was keeping this code from getting
hit.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1447: ASSERT(currentCount == 0);
On 2016/06/24 17:05:27, Ben Wagner wrote:
> Wouldn't this assert trigger for e.g. "mat2(a, vec3(b, c, d))"?
> 
> However, see comment in IRGenerator::convertConstructor.

This is an error -- the IRGenerator shouldn't be allowing as many cases through
as it does.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1449: currentCount = 0;
On 2016/06/24 17:05:28, Ben Wagner wrote:
> Isn't this incorrect for e.g. "mat2(vec3(a, b, c), d)"?
> 
> However, see comment in IRGenerator::convertConstructor.

Same, this isn't actually legal.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1472:
this->writeOpCode(SpvOpCompositeConstruct, 3 + (int32_t) c.fArguments.size(),
out);
On 2016/06/24 17:05:28, Ben Wagner wrote:
> Is this case for arrays?

Array construction isn't supported at the moment.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1543: SpvId result = this->nextId();
On 2016/06/24 17:05:28, Ben Wagner wrote:
> This doesn't seem right. If we're asking for an lvalue, most likely there's an
> lvalue at the source level, so creating an anonymous temporary means that the
> actual lvalue is not being assigned.
> 
> One reason this might come up is due to the out param issue I commented on in
> IRGenerator.

I'm aware of that, but it is necessary because of the out parameter workaround I
reference in several spots in this file. An actual error (i.e. using '5' as an
out parameter) would have been caught in IRGenerator.

[dogben, 2016-06-26 03:21:54]

Finished with a full pass over the code. Haven't looked at diffs from previous
yet.

Following up on a few related comments:

On 2016/06/24 at 21:23:12, ethannicholas wrote:
...
> I might end up changing this, but in general it is not my intent to slavishly
adhere to the GLSL spec. This is not a GLSL compiler, and the language will
become more different as we proceed.
...
> Non-square matrices are on my list; obviously there's a lot of the groundwork
laid, but they don't actually work (and we don't use them).
...
> We don't use them. I've got it on my list of things to add support for.
...
> I only officially support float vectors and matrices at the moment (and this
is all Skia uses), so there's never actually any coercion for them. Obviously
I've laid a lot of the groundwork for other types, but I'm not worried about
having it all perfect in the first release.
...

If SkSL is intended to diverge from GLSL, is there any point in implementing
features that aren't used in Skia? Could we just remove/disable them from the
lexer and parser? (And maybe add asserts in the no-longer-possible cases in
IR/Code Generator?)

Even if the SkSL divergence is only adding additional features that are
backwards-compatible with GLSL and the compiler will eventually support all of
GLSL, I have to wonder if it wouldn't be better to start with a robust
implementation of the limited features used in Skia, rather than having certain
features that don't give a compiler error but yet aren't actually implemented.

[dogben, 2016-06-26 03:57:56]

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

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:315: return fColumns * this->alignment();
On 2016/06/24 at 21:23:07, ethannicholas wrote:
> On 2016/06/21 21:52:51, Ben Wagner wrote:
> > Shouldn't this be fColumns * (fComponentType->size() rounded up to the next
> > fComponentType->alignment())?
> 
> You'd think so, but no. An OpenGL float[] is 16-byte aligned, and each
individual float consumes 16 bytes.

Will all targets use the same layout as OpenGL? (And is SPIR-V specific to
OpenGL?)

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:153: std::vector<std::unique_ptr<Expression>>
currentVarSizes;
On 2016/06/24 at 21:23:08, ethannicholas wrote:
> On 2016/06/23 15:25:08, Ben Wagner wrote:
> > If baseType is an array type, should the sizes be added to currentVarSizes
> > before iterating over fSizes?
> 
> baseType can't be an array type, since it's the sizes that make array types.
But I'll add an assert just to make that clear.

The GLSL spec contains these example var declaration:
vec4[2] a[3]; // size-3 array of size-2 array of vec4
vec4[3][2] a; // size-3 array of size-2 array of vec4

I'm happy to not support this, but that might affect function declarations with
array parameters.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:676: if
(!determine_binary_type(expression.fOperator, left->fType, right->fType,
&leftType,
On 2016/06/24 at 21:23:09, ethannicholas wrote:
> On 2016/06/23 15:25:10, Ben Wagner wrote:
> > Seems like we need special handling for logical operators (&&, ||).
> 
> In what way?

Sorry for being unclear.

Is "an_int || another_int" legal? If so, both sides should be coerced to bool.
Using determine_binary_type would allow both sides to remain ints.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:677: &rightType, &resultType, true)) {
On 2016/06/24 at 21:23:08, ethannicholas wrote:
> On 2016/06/23 15:25:11, Ben Wagner wrote:
> > tryFlipped probably shouldn't be true for shift operations.
> 
> It doesn't actually flip the operation around, it just determines the types as
if it had (so I don't have to duplicate the block of code that checks left
coercing to right and matrix * scalar).

Understood. This comment really depends on what you do for shift operations for
the FIXME above, but since we need to check that rhs is an int, I don't see how
tryFlipped would work.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLConstr...
File src/sksl/ir/SkSLConstructor.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLConstr...
src/sksl/ir/SkSLConstructor.h:20: std::vector<std::unique_ptr<Expression>>
parameters)
On 2016/06/24 at 21:23:09, ethannicholas wrote:
> On 2016/06/22 17:43:57, Ben Wagner wrote:
> > nit: s/parameters/arguments/g
> 
> Wow. I had always thought those were interchangeable synonyms. TIL.

In 99% of contexts, I use them interchangeably. It's just easier to understand
compiler code if you use different terms for params vs. args.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLSwizzle.h
File src/sksl/ir/SkSLSwizzle.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLSwizzl...
src/sksl/ir/SkSLSwizzle.h:21: static std::shared_ptr<Type> get_type(Expression&
value,
On 2016/06/24 at 21:23:10, ethannicholas wrote:
> On 2016/06/22 17:43:57, Ben Wagner wrote:
> > nit: I'm used to all references being const, but I don't know what style
Skia
> > normally uses.
> 
> Normally I would have used const, but all of these types are immutable in the
first place -- does it matter?

Nope. :-)

The style I'm used to is that everything that can be const, is const; and all
reference parameters are const -- mutable parameters must be pointers. The
benefit is that (in most cases) the call site has an indication of whether the
callee can mutate the parameters. E.g.:
string x;
foo(&x); // foo could mutate x. (In most cases, this means foo *should* mutate
x.)
bar(x);  // bar can't mutate x.

To follow up on a previous comment, using "mutable" breaks the contract in this
style. It's still used for caches, where the externally-visible constness of the
object is preserved.

Again, this is just the style I'm used to. I wanted to give a little bit of
background for where some of my comments are coming from.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLSwizzl...
src/sksl/ir/SkSLSwizzle.h:29: case 1: return kFloat_Type;
On 2016/06/24 at 21:23:10, ethannicholas wrote:
> On 2016/06/22 17:43:57, Ben Wagner wrote:
> > nit: Aren't all the "case 1"s in this function redundant?
> 
> What do you mean?

Sorry for being unclear. The "count == 1" case is handled above.

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

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:32: std::string description() {
On 2016/06/24 at 21:23:10, ethannicholas wrote:
> On 2016/06/22 17:43:58, Ben Wagner wrote:
> > override? const?
> 
> You are correct that it should have been const, but it's not overriding
anything.

Class hierarchy is Type -> Symbol -> IRNode. IRNode has a virtual description
method.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:164: count = ((IntLiteral&) *size).fValue;
nit: Maybe check that count is sane?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:506: return std::unique_ptr<Expression>(new
IntLiteral(expr.fPosition,
nit: Maybe check that fValue is in range of uint?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:865: }
On 2016/06/24 at 21:23:10, ethannicholas wrote:
> On 2016/06/24 17:05:27, Ben Wagner wrote:
> > Seems that you should check that the argument is a number if it's not a
bool.
> 
> Why? If we're doing a number->number cast, it just ends up creating a
Constructor below, which is what we want.

Ok, in that case, should we be checking that the argument types match the
parameter types for all constructor calls, not just structs?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:894: return std::unique_ptr<Expression>(new
IntLiteral(base->fPosition,
nit: Maybe check that -fValue is in range of int?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:1016: return std::unique_ptr<Expression>(new
Swizzle(std::move(base), swizzleComponents));
nit: I assume the following is illegal:
vec2 a = vec2(1, 2);
a.xx = vec2(3, 4);

Should we check for that case here? Or maybe in markWrittenTo?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
File src/sksl/SkSLSPIRVCodeGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1324:
arguments.push_back(this->getLValue(*c.fArguments[i], out));
Are swizzles not allowed as out args? Is that checked in IRGenerator?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1447: ASSERT(currentCount == 0);
On 2016/06/24 at 21:23:11, ethannicholas wrote:
> On 2016/06/24 17:05:27, Ben Wagner wrote:
> > Wouldn't this assert trigger for e.g. "mat2(a, vec3(b, c, d))"?
> > 
> > However, see comment in IRGenerator::convertConstructor.
> 
> This is an error -- the IRGenerator shouldn't be allowing as many cases
through as it does.

(My reading of the GLSL spec says this is legal, but I'm more than happy not to
support it.)

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1517: SpvId base =
this->getLValue(*index.fBase, out);;
typo: double ;

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1524: this->writeExpression(*index.fIndex,
out),
Double-checking: this wants array index, not offset?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1530: SpvId base = this->getLValue(*f.fBase,
out);;
typo: double ;

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1538: this->writeIntLiteral(index),
Double-checking: this wants field index, not offset?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1543: SpvId result = this->nextId();
On 2016/06/24 at 21:23:10, ethannicholas wrote:
> On 2016/06/24 17:05:28, Ben Wagner wrote:
> > This doesn't seem right. If we're asking for an lvalue, most likely there's
an
> > lvalue at the source level, so creating an anonymous temporary means that
the
> > actual lvalue is not being assigned.
> > 
> > One reason this might come up is due to the out param issue I commented on
in
> > IRGenerator.
> 
> I'm aware of that, but it is necessary because of the out parameter workaround
I reference in several spots in this file. An actual error (i.e. using '5' as an
out parameter) would have been caught in IRGenerator.

I see. How do you feel about adding "ASSERT(outParamHack)" here?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1572: this->writeIntLiteral(index),
Double-checking: this wants vector index, not offset?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1584: for (int i = 0; i <
baseType->columns(); i++) {
nit: Add comment explaining that offset is the index into the concatenation of
lhs and value?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1609: this->writeInstruction(SpvOpLoad,
this->getType(ref.fVariable->fType), result, var, out);
(Mostly food for thought, maybe future improvements.)

Does the SPIR-V-to-native compiler optimize away unneeded variables? Does it
automatically put things on the heap when the number of temporaries exceeds the
number of registers?

Since GLSL doesn't support recursion or pointers, ISTM that there's no need to
explicitly use the stack or the heap. Everything could be in (virtual)
registers.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1613: std::vector<SpvId>
SPIRVCodeGenerator::getAccessChain(Expression& expr, std::ostream& out) {
Isn't this very similar to getLValue? Why different implementations for lvalues
vs. rvalues?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1708: this->storeToLValue(*b.fLeft, rhs,
out);
Spec says, "Expressions on the left of an assignment are evaluated before
expressions on the right of the assignment." I assume this matters for something
like "a[i++] = i++;"

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1711: case Token::LOGICALAND:
How do LOGICALANDEQ and LOGICALOREQ work? Do both operands get evaluated?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1720: std::shared_ptr<Type> resultType =
b.fType;
nit: const ref

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1724: std::shared_ptr<Type> operandType;
nit: Type*

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1765: this->storeToLValue(*b.fLeft, result,
out);
This will evaluate sub-expressions of b.fLeft twice. Same for all *EQ operations
in this method.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1777:
this->writeInstruction(SpvOpMatrixTimesScalar, this->getType(b.fType), result,
rhs,
nit: Is switching the argument order allowed for "precise"?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1877: ABORT("unsupported binary expression:
%s\n", b.description().c_str());
I don't see bitwise operators, LOGICALANDEQ, or LOGICALOREQ here. Are those
TODOs?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1976: SpvId result =
this->writeBinaryOperation(p.fType, p.fType, expr, one, SpvOpFAdd,
"one" is not the correct type for SpvOpFAdd. Same for MINUSMINUS and
SPIRVCodeGenerator::writePostfixExpression.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1977: SpvOpIAdd, SpvOpIAdd, SpvOpUndef,
out);
nit: odd indentation

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1978: this->storeToLValue(*p.fOperand,
result, out);
Sub-expressions in fOperand are evaluated twice. Same for MINUSMINUS and
SPIRVCodeGenerator::writePostfixExpression.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1985: SpvOpISub, SpvOpISub, SpvOpUndef,
out);
nit: odd indentation

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2042: if (i.fType == kInt_Type) {
It looks like IntLiteral::fType is always initialized to kInt_Type. Is that a
TODO?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2067: if (f.fType == kFloat_Type) {
It looks like FloatLiteral::fType is always initialized to kFloat_type. Is that
a TODO?

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2074: memcpy(&bits, &value, sizeof(bits));
Is this platform-dependent?

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
File src/sksl/SkSLSPIRVCodeGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:864: this->writeOpCode(opCode, 1 + (length +
4) / 4, out);
nit: maybe define int32_t words_for_string(const char* x) { return (strlen(x) +
4) / 4; }?

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1351:
arguments.push_back(this->writeExpression(*c.fArguments[i], fConstantBuffer));
nit: Maybe assert that all arguments are scalar type? (isConstant can return
true for non-literals.)

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1388: }
nit: else assert?

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1406: }
nit: else assert?

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1423: // with a single argument, a matrix
will have all of its diagonal entries equal to the
nit: Assert argument is a scalar?

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1452: ASSERT(currentCount == 0);
nit: Also assert vector.rows == matrix.rows?

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1492: if (arguments.size() == 1 &&
c.fArguments[0]->fType->kind() == Type::kScalar_Kind) {
nit: I think the scalar check should just be an assert for now.

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1601: this->writeInstruction(SpvOpStore, id,
value, out);
Does SPIR-V automatically handle copying composites on SpvOpStore, or will this
just create an alias? Example:
vec2 a = vec2(1, 2);
vec2 b = a;
b.y = 3;
if (a == b) {
  bug();
}

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2236:
this->writeInstruction(SpvOpTypePointer, ptrType, storageClass, type,
fConstantBuffer);
nit: Any reason this isn't this->getPointerType()?

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2237: this->writeInstruction(SpvOpVariable,
ptrType, result, storageClass, fConstantBuffer);
Is fConstantBuffer correct?

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2245: if (!decl.fVars[i]->fIsReadFrom &&
!decl.fVars[i]->fIsWrittenTo) {
nit: Should this be checked in IRGenerator?

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2265: this->writeInstruction(SpvOpVariable,
type, id, storageClass, fConstantBuffer);
Is fConstantBuffer correct?

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2274: ASSERT(!fCurrentBlock);
nit: wrong indentation

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2275: fCurrentBlock = -1;
nit: Add comment that this is to avoid the asserts in writeOpCode?

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2349: if (fCurrentBlock) {
nit: Maybe add a comment to explain fCurrentBlock could be 0 due to return.

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2384: this->writeInstruction(SpvOpBranch,
start, out);
nit: wrong indentation

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2429: interfaceVars.push_back(id);
Doesn't this end up adding the same id twice, once here and once below?

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2444: for (auto entry : fFunctionMap) {
nit: const ref

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2445: if (entry.first->fName == "main") {
nit: wrong indentation

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2450: for (auto entry : fVariableMap) {
nit: const ref

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2493: out << fExternalFunctionsBuffer.str();
fExternalFunctionsBuffer doesn't seem to be used anywhere.

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2495: }
nit: Maybe assert that fGlobalInitializersBuffer and fVariableBuffer are empty?

[ethannicholas, 2016-06-27 20:35:04]

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

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:315: return fColumns * this->alignment();
On 2016/06/26 03:57:52, Ben Wagner wrote:
> On 2016/06/24 at 21:23:07, ethannicholas wrote:
> > On 2016/06/21 21:52:51, Ben Wagner wrote:
> > > Shouldn't this be fColumns * (fComponentType->size() rounded up to the
next
> > > fComponentType->alignment())?
> > 
> > You'd think so, but no. An OpenGL float[] is 16-byte aligned, and each
> individual float consumes 16 bytes.
> 
> Will all targets use the same layout as OpenGL? 

At least for the foreseeable future. If that ends up changing, my plan is to
delegate size(), alignment(), etc. to a class which handles the memory layout
model.

> (And is SPIR-V specific to OpenGL?)

SPIR-V is specific to Vulkan, but it has a strong OpenGL heritage.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:153: std::vector<std::unique_ptr<Expression>>
currentVarSizes;
On 2016/06/26 03:57:52, Ben Wagner wrote:
> On 2016/06/24 at 21:23:08, ethannicholas wrote:
> > On 2016/06/23 15:25:08, Ben Wagner wrote:
> > > If baseType is an array type, should the sizes be added to currentVarSizes
> > > before iterating over fSizes?
> > 
> > baseType can't be an array type, since it's the sizes that make array types.
> But I'll add an assert just to make that clear.
> 
> The GLSL spec contains these example var declaration:
> vec4[2] a[3]; // size-3 array of size-2 array of vec4
> vec4[3][2] a; // size-3 array of size-2 array of vec4
> 
> I'm happy to not support this, but that might affect function declarations
with
> array parameters.

Skia (and every shader I have discovered in the wild) exclusively uses the
C-style 'vec4 v[3]' declaration, so that is what I support for the moment. I
actually prefer the 'vec4[3] v' model, and may push for a change to that in the
future, but I have no intention of supporting a mixture of the two.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:676: if
(!determine_binary_type(expression.fOperator, left->fType, right->fType,
&leftType,
On 2016/06/26 03:57:52, Ben Wagner wrote:
> On 2016/06/24 at 21:23:09, ethannicholas wrote:
> > On 2016/06/23 15:25:10, Ben Wagner wrote:
> > > Seems like we need special handling for logical operators (&&, ||).
> > 
> > In what way?
> 
> Sorry for being unclear.
> 
> Is "an_int || another_int" legal? If so, both sides should be coerced to bool.
> Using determine_binary_type would allow both sides to remain ints.

No, int cannot be coerced to bool.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLSwizzle.h
File src/sksl/ir/SkSLSwizzle.h (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLSwizzl...
src/sksl/ir/SkSLSwizzle.h:29: case 1: return kFloat_Type;
On 2016/06/26 03:57:53, Ben Wagner wrote:
> On 2016/06/24 at 21:23:10, ethannicholas wrote:
> > On 2016/06/22 17:43:57, Ben Wagner wrote:
> > > nit: Aren't all the "case 1"s in this function redundant?
> > 
> > What do you mean?
> 
> Sorry for being unclear. The "count == 1" case is handled above.

Derp. Yes, it is.

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

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:32: std::string description() {
On 2016/06/26 03:57:53, Ben Wagner wrote:
> On 2016/06/24 at 21:23:10, ethannicholas wrote:
> > On 2016/06/22 17:43:58, Ben Wagner wrote:
> > > override? const?
> > 
> > You are correct that it should have been const, but it's not overriding
> anything.
> 
> Class hierarchy is Type -> Symbol -> IRNode. IRNode has a virtual description
> method.

Well, sure, but aren't we talking about Field, not Type? Field doesn't extend
anything.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:865: }
On 2016/06/26 03:57:53, Ben Wagner wrote:
> On 2016/06/24 at 21:23:10, ethannicholas wrote:
> > On 2016/06/24 17:05:27, Ben Wagner wrote:
> > > Seems that you should check that the argument is a number if it's not a
> bool.
> > 
> > Why? If we're doing a number->number cast, it just ends up creating a
> Constructor below, which is what we want.
> 
> Ok, in that case, should we be checking that the argument types match the
> parameter types for all constructor calls, not just structs?

I have tightened this up considerably.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:1016: return std::unique_ptr<Expression>(new
Swizzle(std::move(base), swizzleComponents));
On 2016/06/26 03:57:53, Ben Wagner wrote:
> nit: I assume the following is illegal:
> vec2 a = vec2(1, 2);
> a.xx = vec2(3, 4);
> 
> Should we check for that case here? Or maybe in markWrittenTo?

I've added a check in markWrittenTo (it's legal on input, illegal on output)

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
File src/sksl/SkSLSPIRVCodeGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1538: this->writeIntLiteral(index),
On 2016/06/26 03:57:54, Ben Wagner wrote:
> Double-checking: this wants field index, not offset?

Correct.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1543: SpvId result = this->nextId();
On 2016/06/26 03:57:55, Ben Wagner wrote:
> On 2016/06/24 at 21:23:10, ethannicholas wrote:
> > On 2016/06/24 17:05:28, Ben Wagner wrote:
> > > This doesn't seem right. If we're asking for an lvalue, most likely
there's
> an
> > > lvalue at the source level, so creating an anonymous temporary means that
> the
> > > actual lvalue is not being assigned.
> > > 
> > > One reason this might come up is due to the out param issue I commented on
> in
> > > IRGenerator.
> > 
> > I'm aware of that, but it is necessary because of the out parameter
workaround
> I reference in several spots in this file. An actual error (i.e. using '5' as
an
> out parameter) would have been caught in IRGenerator.
> 
> I see. How do you feel about adding "ASSERT(outParamHack)" here?

I don't see it as necessary, honestly -- if we were ever able to turn this hack
off and failed to update this spot, the broken calls would be instantly caught
(by the driver or spirv-val) as a mismatch between argument and parameter types.
It's not like assembly where it would just crash inexplicably; it would be very
obvious what the problem was.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1572: this->writeIntLiteral(index),
On 2016/06/26 03:57:53, Ben Wagner wrote:
> Double-checking: this wants vector index, not offset?

Correct.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1584: for (int i = 0; i <
baseType->columns(); i++) {
On 2016/06/26 03:57:54, Ben Wagner wrote:
> nit: Add comment explaining that offset is the index into the concatenation of
> lhs and value?

Sorry, this desperately needed comments. Fixed.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1609: this->writeInstruction(SpvOpLoad,
this->getType(ref.fVariable->fType), result, var, out);
On 2016/06/26 03:57:55, Ben Wagner wrote:
> (Mostly food for thought, maybe future improvements.)
> 
> Does the SPIR-V-to-native compiler optimize away unneeded variables? Does it
> automatically put things on the heap when the number of temporaries exceeds
the
> number of registers?
> 
> Since GLSL doesn't support recursion or pointers, ISTM that there's no need to
> explicitly use the stack or the heap. Everything could be in (virtual)
> registers.

I assume that that is in fact how it is implemented, but have no actual insight
into what is happening on the backend. I can only say that the code I produce is
generally exactly equivalent to the code the reference compiler produces.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1613: std::vector<SpvId>
SPIRVCodeGenerator::getAccessChain(Expression& expr, std::ostream& out) {
On 2016/06/26 03:57:55, Ben Wagner wrote:
> Isn't this very similar to getLValue? Why different implementations for
lvalues
> vs. rvalues?

They started out considerably more different than that (using
OpCompositeExtract, among other differences, in the rvalue case) until I found
that Adreno got really crashy unless I did it this way, and I didn't realize how
close the two cases had gotten over time. I've added a TODO to look into
unifying them.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1708: this->storeToLValue(*b.fLeft, rhs,
out);
On 2016/06/26 03:57:55, Ben Wagner wrote:
> Spec says, "Expressions on the left of an assignment are evaluated before
> expressions on the right of the assignment." I assume this matters for
something
> like "a[i++] = i++;"

My intent was to leave expression evaluation order undefined for the time being,
a la C.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1711: case Token::LOGICALAND:
On 2016/06/26 03:57:54, Ben Wagner wrote:
> How do LOGICALANDEQ and LOGICALOREQ work? Do both operands get evaluated?

Currently it fails with an "unsupported binary operation" assert.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1777:
this->writeInstruction(SpvOpMatrixTimesScalar, this->getType(b.fType), result,
rhs,
On 2016/06/26 03:57:54, Ben Wagner wrote:
> nit: Is switching the argument order allowed for "precise"?

There's no other (sane) way to do this, as there's OpMatrixTimesScalar but not
OpScalarTimesMatrix.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1877: ABORT("unsupported binary expression:
%s\n", b.description().c_str());
On 2016/06/26 03:57:54, Ben Wagner wrote:
> I don't see bitwise operators, LOGICALANDEQ, or LOGICALOREQ here. Are those
> TODOs?

Yes, I've added a comment to that effect.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1976: SpvId result =
this->writeBinaryOperation(p.fType, p.fType, expr, one, SpvOpFAdd,
On 2016/06/26 03:57:55, Ben Wagner wrote:
> "one" is not the correct type for SpvOpFAdd. Same for MINUSMINUS and
> SPIRVCodeGenerator::writePostfixExpression.

Fixed.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1978: this->storeToLValue(*p.fOperand,
result, out);
On 2016/06/26 03:57:55, Ben Wagner wrote:
> Sub-expressions in fOperand are evaluated twice. Same for MINUSMINUS and
> SPIRVCodeGenerator::writePostfixExpression.

Ack. Fix soon.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2042: if (i.fType == kInt_Type) {
On 2016/06/26 03:57:54, Ben Wagner wrote:
> It looks like IntLiteral::fType is always initialized to kInt_Type. Is that a
> TODO?

Eventually we'll have support for UInt, and possibly even different sizes of
int, so this was written in anticipation of that.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:2074: memcpy(&bits, &value, sizeof(bits));
On 2016/06/26 03:57:55, Ben Wagner wrote:
> Is this platform-dependent?

The C standard is flexible enough that there's probably some insane thing you
could do to make this break (like having endianness differ between uint32_t and
float... is that legal?), but I think this should work on any reasonable C.

[dogben, 2016-06-28 02:14:56]

Getting very close to LGTM!

Lots of nits that you can ignore below.

https://codereview.chromium.org/1984363002/diff/320001/src/gpu/vk/GrVkPipelin...
File src/gpu/vk/GrVkPipelineStateBuilder.cpp (right):

https://codereview.chromium.org/1984363002/diff/320001/src/gpu/vk/GrVkPipelin...
src/gpu/vk/GrVkPipelineStateBuilder.cpp:103: std::stringstream buffer;
On 2016/06/20 17:45:49, ethannicholas wrote:
> On 2016/06/20 16:23:17, Ben Wagner wrote:
> > Unused?
> 
> A bit of debug code I failed to remove. Oops!

(Still there.)

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

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:136: return fName == other.fName;
On 2016/06/21 21:52:52, Ben Wagner wrote:
> Isn't the following legal?
> struct same {
>   int x;
> } a;
> 
> void f () {
>   struct same {
>     float y;
>   } b;
> }
> 
> It seems that just comparing name is not sufficient.

Maybe "this == &other" would work?

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:246: size_t alignment() {
On 2016/06/24 21:23:07, ethannicholas wrote:
> On 2016/06/21 21:52:52, Ben Wagner wrote:
> > I don't understand a few of these calculations.
> 
> That's because they're stupid, but based on the spec. 7.6.2.2: Standard
Uniform
> Block Layout. I added a comment to make that clear.

It would be great if someone who knows how Skia sets up memory for shader input
could take a look at alignment(), stride(), and size().

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:676: if
(!determine_binary_type(expression.fOperator, left->fType, right->fType,
&leftType,
On 2016/06/27 20:35:03, ethannicholas wrote:
> On 2016/06/26 03:57:52, Ben Wagner wrote:
> > On 2016/06/24 at 21:23:09, ethannicholas wrote:
> > > On 2016/06/23 15:25:10, Ben Wagner wrote:
> > > > Seems like we need special handling for logical operators (&&, ||).
> > > 
> > > In what way?
> > 
> > Sorry for being unclear.
> > 
> > Is "an_int || another_int" legal? If so, both sides should be coerced to
bool.
> > Using determine_binary_type would allow both sides to remain ints.
> 
> No, int cannot be coerced to bool.

Sorry for being unclear. If you give "an_int || another_int" to
determine_binary_type, it will happily tell you that leftType, rightType, and
resultType are kInt_Type.

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

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:32: std::string description() {
On 2016/06/27 20:35:03, ethannicholas wrote:
> On 2016/06/26 03:57:53, Ben Wagner wrote:
> > On 2016/06/24 at 21:23:10, ethannicholas wrote:
> > > On 2016/06/22 17:43:58, Ben Wagner wrote:
> > > > override? const?
> > > 
> > > You are correct that it should have been const, but it's not overriding
> > anything.
> > 
> > Class hierarchy is Type -> Symbol -> IRNode. IRNode has a virtual
description
> > method.
> 
> Well, sure, but aren't we talking about Field, not Type? Field doesn't extend
> anything.

Gah! Sorry.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
File src/sksl/SkSLSPIRVCodeGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1324:
arguments.push_back(this->getLValue(*c.fArguments[i], out));
On 2016/06/26 03:57:55, Ben Wagner wrote:
> Are swizzles not allowed as out args? Is that checked in IRGenerator?

(Asking because getLValue doesn't handle swizzles.)

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:546:
std::static_pointer_cast<FunctionDeclaration>(std::move(result))
Sorry, my mistake. std::move doesn't do anything in this case.

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.h (right):

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.h:71: std::vector<std::unique_ptr<Expression>>
argumentss);
typo: argumentss

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLParser.cpp
File src/sksl/SkSLParser.cpp (right):

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:258: /* modifiers type IDENTIFIER (EQ expression)?
(COMMA IDENTIFER (EQ expression)?)* SEMICOLON */
nit: should be "modifiers type IDENTIFIER varDeclarationEnd

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:298: type = std::shared_ptr<Type>(new Type(name,
Type::kArray_Kind, type,
nit:
type.reset(new Type(name, Type::kArray_Kind, std::move(type), (int) columns));

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:302: std::shared_ptr<Type>(type)));
nit: s/std::shared_ptr<Type>(type)/std::move(type)/

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:317: /* structDeclaration varDeclarationEnd */
nit: Should be "structDeclaration ((IDENTIFIER varDeclarationEnd) | SEMICOLON)"

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLCompiler.cpp
File src/sksl/SkSLCompiler.cpp (right):

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.cpp:46: static auto types =
std::shared_ptr<SymbolTable>(new SymbolTable(nullptr));
I can't figure out what this is doing. I don't see a SymbolTable constructor
that would accept nullptr.

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:890: fErrors.error(position, "invalid argument to
'" + type->description() +
nit: position for "invalid argument" error should be the position of the
argument, not the constructor. (Several places in this method.)

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:907: if (type->kind() != Type::kScalar_Kind) {
nit: This condition is always true.

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:912: "parameter to '" + type->description() + "'
constructor");
nit: s/parameter/argument/

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:918: !((kind == Type::kVector_Kind || kind ==
Type::kMatrix_Kind) && (actual == 1))) {
nit: condition can be simplified, since we know it's a vector or matrix.

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:921: (min == 1 ? "" : "s") + ", but found " +
to_string(actual) +
nit: min will never be 1

(Maybe you were planning to reuse this code for arrays, but I don't think that
would work well.)

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:1175: fErrors.error(expr.fPosition, "cannot write
to the same swizzle field more than "
uber-nit: line break after comma instead of mid-string?

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLSPIRVCode...
File src/sksl/SkSLSPIRVCodeGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1013: auto entry =
fTypeMap.find(type.name());
nit: const ref

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1090: auto entry = fTypeMap.find(key);
nit: const ref

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1624: // (4, 1, 5).
3, 1, 4

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1844: ASSERT(*operandType ==
*b.fRight->fType.get());
nit: .get() is superfluous

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLSymbol...
File src/sksl/ir/SkSLSymbolTable.cpp (right):

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.cpp:18: return
std::static_pointer_cast<UnresolvedFunction>(s)->fFunctions;
nit: ((UnresolvedFunction*)s.get())->fFunctions avoids creating a temporary
shared_ptr.

One more below, too.

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.cpp:25: auto entry = fSymbols.find(name);
nit: const ref

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.cpp:39: for (std::shared_ptr<FunctionDeclaration>
prev : previousFunctions) {
nit: const ref

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.cpp:41: for (std::shared_ptr<FunctionDeclaration>
current : functions) {
nit: const ref

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.cpp:63: auto existing = fSymbols.find(name);
nit: const ref

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.cpp:67: std::shared_ptr<Symbol> oldSymbol =
existing->second;
nit: const ref

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.cpp:72: fSymbols[name] = std::shared_ptr<Symbol>(new
UnresolvedFunction(functions));
nit: fSymbols[name].reset(new UnresolvedFunction(std::move(functions)));

Same below.

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLSymbol...
src/sksl/ir/SkSLSymbolTable.cpp:75: for (auto f :
std::static_pointer_cast<UnresolvedFunction>(oldSymbol)->fFunctions) {
nit: const ref

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

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:57: , fIsNumber(false)
nit: Given that there are so many constructors and so many fields, I think it
would be more readable to put default initializers on the fields and only
include the fields in the constructor that are non-default.

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:125: : INHERITED(Position(), kType_Kind, std::move(name))
nit: maybe delegate to next constructor?

https://codereview.chromium.org/1984363002/diff/460001/tests/SkSLErrorTest.cpp
File tests/SkSLErrorTest.cpp (right):

https://codereview.chromium.org/1984363002/diff/460001/tests/SkSLErrorTest.cp...
tests/SkSLErrorTest.cpp:1: /*
IWBN to add tests for more of the newly-added errors.

[dogben, 2016-06-28 03:20:01]

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
File src/sksl/SkSLSPIRVCodeGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1601: this->writeInstruction(SpvOpStore, id,
value, out);
On 2016/06/26 03:57:56, Ben Wagner wrote:
> Does SPIR-V automatically handle copying composites on SpvOpStore, or will
this
> just create an alias? Example:
> vec2 a = vec2(1, 2);
> vec2 b = a;
> b.y = 3;
> if (a == b) {
>   bug();
> }

Another example:
float foo(vec2 c) {
  c.x++;
  return c.x + c.y;
}

void main() {
  vec2 d = vec2(3.0, 4.0);
  foo(d);
  if (d.x != 3.0) {
    bug();
  }
}

(If there's no copying, then due to all parameters being pointerized, c is an
alias for d, and modifying c will modify d as well.)

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:1168: ((VariableReference&)
expr).fVariable->fIsWrittenTo = true;
Check if variable (or field?) is const or uniform?

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLField.h
File src/sksl/ir/SkSLField.h (right):

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLField....
src/sksl/ir/SkSLField.h:23: struct Field : public Symbol {
nit: maybe rename InterfaceField?

[ethannicholas, 2016-06-28 14:51:26]

Description was changed from

==========
initial checkin of SkSL compiler

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
initial checkin of SkSL compiler

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[ethannicholas, 2016-06-28 14:51:26]

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

[egdaniel, 2016-06-28 15:53:32]

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

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:306: return (fRows + fRows % 2) * fComponentType->size();
This seems wrong. A column matrix should be though of as an array of size
fColumns of vec<fRows>. Thus this should have a base alignment equal to an array
of vec<fRow>

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:310: return fComponentType->alignment() * 4;
This should be the base alignment of the scalar rounded up to 16. If the size of
the scalar is < 4 then this is incorrect. If you want to assume the alignment is
>=4 then I think we need some assert here or in scalar->size().

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:312: return fComponentType->componentType()->size() * 4;
same need for asserts as above

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:341: // least) there is no actual reason to require a
stride of 16, provided that both the shader
This is actually all required by the Vulkan spec even if SPIR-V supports other
layouts

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:346: // for the time being, we only handle 32 bit numbers
is this asserted somewhere?

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:349: return std::max(this->componentType()->size(),
(size_t) 16);
If we are trying to match the vulkan spec here, the stride of an array and
matrix must be an integer multiple of the base alignment.

Just set this to alignment

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:365: return fColumns * fComponentType->size();
does a vec3 not have a size of a vec4?

[ethannicholas, 2016-06-30 15:19:28]

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

https://codereview.chromium.org/1984363002/diff/360001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:136: return fName == other.fName;
On 2016/06/28 02:14:54, Ben Wagner wrote:
> On 2016/06/21 21:52:52, Ben Wagner wrote:
> > Isn't the following legal?
> > struct same {
> >   int x;
> > } a;
> > 
> > void f () {
> >   struct same {
> >     float y;
> >   } b;
> > }
> > 
> > It seems that just comparing name is not sufficient.
> 
> Maybe "this == &other" would work?

The struct declaration inside a function is not currently legal (there's really
no point to something like that in a shader, though obviously it wouldn't be
hard to add support), so for the time being simply comparing names should be
sufficient.

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/380001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:676: if
(!determine_binary_type(expression.fOperator, left->fType, right->fType,
&leftType,
On 2016/06/28 02:14:54, Ben Wagner wrote:
> On 2016/06/27 20:35:03, ethannicholas wrote:
> > On 2016/06/26 03:57:52, Ben Wagner wrote:
> > > On 2016/06/24 at 21:23:09, ethannicholas wrote:
> > > > On 2016/06/23 15:25:10, Ben Wagner wrote:
> > > > > Seems like we need special handling for logical operators (&&, ||).
> > > > 
> > > > In what way?
> > > 
> > > Sorry for being unclear.
> > > 
> > > Is "an_int || another_int" legal? If so, both sides should be coerced to
> bool.
> > > Using determine_binary_type would allow both sides to remain ints.
> > 
> > No, int cannot be coerced to bool.
> 
> Sorry for being unclear. If you give "an_int || another_int" to
> determine_binary_type, it will happily tell you that leftType, rightType, and
> resultType are kInt_Type.

Sorry, gotcha.

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
File src/sksl/SkSLSPIRVCodeGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/400001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1324:
arguments.push_back(this->getLValue(*c.fArguments[i], out));
On 2016/06/28 02:14:54, Ben Wagner wrote:
> On 2016/06/26 03:57:55, Ben Wagner wrote:
> > Are swizzles not allowed as out args? Is that checked in IRGenerator?
> 
> (Asking because getLValue doesn't handle swizzles.)

This whole mess was revamped and works properly now.

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLParser.cpp
File src/sksl/SkSLParser.cpp (right):

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:302: std::shared_ptr<Type>(type)));
On 2016/06/28 02:14:54, Ben Wagner wrote:
> nit: s/std::shared_ptr<Type>(type)/std::move(type)/

Ack! That's not a nit, that's completely broken! Thanks for the catch.

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
File src/sksl/SkSLSPIRVCodeGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1601: this->writeInstruction(SpvOpStore, id,
value, out);
On 2016/06/26 03:57:56, Ben Wagner wrote:
> Does SPIR-V automatically handle copying composites on SpvOpStore, or will
this
> just create an alias? Example:
> vec2 a = vec2(1, 2);
> vec2 b = a;
> b.y = 3;
> if (a == b) {
>   bug();
> }

Loading a and storing it in b will duplicate the vector, so they are
independent.

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1601: this->writeInstruction(SpvOpStore, id,
value, out);
On 2016/06/28 03:20:01, Ben Wagner wrote:
> On 2016/06/26 03:57:56, Ben Wagner wrote:
> > Does SPIR-V automatically handle copying composites on SpvOpStore, or will
> this
> > just create an alias? Example:
> > vec2 a = vec2(1, 2);
> > vec2 b = a;
> > b.y = 3;
> > if (a == b) {
> >   bug();
> > }
> 
> Another example:
> float foo(vec2 c) {
>   c.x++;
>   return c.x + c.y;
> }
> 
> void main() {
>   vec2 d = vec2(3.0, 4.0);
>   foo(d);
>   if (d.x != 3.0) {
>     bug();
>   }
> }
> 
> (If there's no copying, then due to all parameters being pointerized, c is an
> alias for d, and modifying c will modify d as well.)

The parameters are pointerized by creating a temporary variable to store them
in. So, yes, the function does end up modifying that temporary variable, but
nobody reads its value so it doesn't matter.

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLCompiler.cpp
File src/sksl/SkSLCompiler.cpp (right):

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLCompiler....
src/sksl/SkSLCompiler.cpp:46: static auto types =
std::shared_ptr<SymbolTable>(new SymbolTable(nullptr));
On 2016/06/28 02:14:54, Ben Wagner wrote:
> I can't figure out what this is doing. I don't see a SymbolTable constructor
> that would accept nullptr.

Well, it *used* to have a constructor that worked for that... which pointed out
to me that somehow I failed to add the SKIA define, so this code was never
getting used. Fixed!

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:1168: ((VariableReference&)
expr).fVariable->fIsWrittenTo = true;
On 2016/06/28 03:20:01, Ben Wagner wrote:
> Check if variable (or field?) is const or uniform?

Done.

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

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:57: , fIsNumber(false)
On 2016/06/28 02:14:55, Ben Wagner wrote:
> nit: Given that there are so many constructors and so many fields, I think it
> would be more readable to put default initializers on the fields and only
> include the fields in the constructor that are non-default.

Last time I did that the reviewer told me not to use default initializers in
Skia... but since you're ok with it, here we go!

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:310: return fComponentType->alignment() * 4;
On 2016/06/28 15:53:32, egdaniel wrote:
> This should be the base alignment of the scalar rounded up to 16. If the size
of
> the scalar is < 4 then this is incorrect. If you want to assume the alignment
is
> >=4 then I think we need some assert here or in scalar->size().

I based mine on the OpenGL spec, which says "rounded up to the base alignment of
a vec4", with various other rule differences. The Vulkan spec says "rounded up
to a multiple of 16". Naturally, since everything is 32 bit, these actually
agree for the time being. The Vulkan spec seems a lot more sensible, though, so
I will follow that.

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:365: return fColumns * fComponentType->size();
On 2016/06/28 15:53:31, egdaniel wrote:
> does a vec3 not have a size of a vec4?

No. It has the *alignment* of a vec4, but not the size. This matters for things
like:

struct foo {
     vec3 bar;
     float baz;
}

baz is offset 12 bytes into the structure, not 16.

[dogben, 2016-06-30 18:19:45]

The new LValue impl looks great!

I have one real comment below ("fColumns and fRows should each be 1"), but
otherwise just nits and commentary.

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLParser.cpp
File src/sksl/SkSLParser.cpp (right):

https://codereview.chromium.org/1984363002/diff/440001/src/sksl/SkSLParser.cp...
src/sksl/SkSLParser.cpp:302: std::shared_ptr<Type>(type)));
On 2016/06/30 15:19:28, ethannicholas wrote:
> On 2016/06/28 02:14:54, Ben Wagner wrote:
> > nit: s/std::shared_ptr<Type>(type)/std::move(type)/
> 
> Ack! That's not a nit, that's completely broken! Thanks for the catch.

std::shared_ptr<Type>(type) is fine -- that's the copy constructor.

std::shared_ptr<Type>(type.get()) would be broken.

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

https://codereview.chromium.org/1984363002/diff/460001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:57: , fIsNumber(false)
On 2016/06/30 15:19:28, ethannicholas wrote:
> On 2016/06/28 02:14:55, Ben Wagner wrote:
> > nit: Given that there are so many constructors and so many fields, I think
it
> > would be more readable to put default initializers on the fields and only
> > include the fields in the constructor that are non-default.
> 
> Last time I did that the reviewer told me not to use default initializers in
> Skia... but since you're ok with it, here we go!

It hasn't been fully supported until recently. I'm a firm believer in default
initializers.

https://codereview.chromium.org/1984363002/diff/520001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/520001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:588: if (*expr->fType == *kInvalid_Type) {
Isn't this unreachable due to the assertion in checkValid? Maybe checkValid
wants to return bool?

https://codereview.chromium.org/1984363002/diff/520001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:1193: this->markWrittenTo(*((FieldAccess&)
expr).fBase);
nit: Is it possible for an interface field to be const/uniform when the base
variable is not? Might need same check here as for variable.

https://codereview.chromium.org/1984363002/diff/520001/src/sksl/SkSLSPIRVCode...
File src/sksl/SkSLSPIRVCodeGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/520001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1317: SpvId tmpVar;
nit: logic might be a bit easier to follow if you just break after
"arguments.push_back(ptr);" In that case, you don't need conditional around
SpvOpVariable.

https://codereview.chromium.org/1984363002/diff/520001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1325: tmpValueId = 0;
nit: also tmpVar = 0

(unless you change this to break)

https://codereview.chromium.org/1984363002/diff/520001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1336: // see getFunctionType for an
explanation of why we're always using out parameters
nit: s/out/pointer/; maybe comment should be before SpvOpVariable below.

https://codereview.chromium.org/1984363002/diff/520001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1734:
this->writeInstruction(SpvOpAccessChain,
very ignorable nit: There are cases where it would be more efficient to use
getAccessChain on swizzle.fBase and add another entry at the end rather than
using getLValue and another SpvOpAccessChain.

https://codereview.chromium.org/1984363002/diff/520001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1829: case Token::PLUSEQ:       // fall
through
nit: include Token::EQ for completeness.

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

https://codereview.chromium.org/1984363002/diff/520001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:76: , fIsNumber(isNumber) {}
fColumns and fRows should each be 1

[ethannicholas, 2016-06-30 19:16:42]

https://codereview.chromium.org/1984363002/diff/520001/src/sksl/SkSLIRGenerat...
File src/sksl/SkSLIRGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/520001/src/sksl/SkSLIRGenerat...
src/sksl/SkSLIRGenerator.cpp:1193: this->markWrittenTo(*((FieldAccess&)
expr).fBase);
On 2016/06/30 18:19:45, Ben Wagner wrote:
> nit: Is it possible for an interface field to be const/uniform when the base
> variable is not? Might need same check here as for variable.

Not legal. I've added a check to error if storage qualifiers are specified on
interface fields.

https://codereview.chromium.org/1984363002/diff/520001/src/sksl/SkSLSPIRVCode...
File src/sksl/SkSLSPIRVCodeGenerator.cpp (right):

https://codereview.chromium.org/1984363002/diff/520001/src/sksl/SkSLSPIRVCode...
src/sksl/SkSLSPIRVCodeGenerator.cpp:1325: tmpValueId = 0;
On 2016/06/30 18:19:45, Ben Wagner wrote:
> nit: also tmpVar = 0
> 
> (unless you change this to break)

tmpVar only gets examined if tmpValueId != 0, though, so it shouldn't matter if
tmpVar is set to anything.

I've taken your advice and just continue the loop to avoid the test, so it's a
moot point.

[dogben, 2016-06-30 19:22:01]

lgtm

LGTM!! Hooray! This is great work!

[egdaniel, 2016-06-30 19:24:15]

gyp and alignment stuff all looks good, minus the one nit

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

https://codereview.chromium.org/1984363002/diff/560001/src/sksl/ir/SkSLType.h...
src/sksl/ir/SkSLType.h:247: return (fColumns + fColumns % 2) *
fComponentType->size();
nit: this logic of (x + x%2) * size is repeated in multiple spots in here. Any
way to pull it out into some sort of shared thing like vector_alignment(entries,
componentSize)?

[ethannicholas, 2016-06-30 20:54:55]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2016-06-30 20:54:57]

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/1984363002/#ps580001
(title: "minor SkSLType refactor")

[commit-bot: I haz the power, 2016-06-30 20:55:07]

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-06-30 21:00:17]

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

[commit-bot: I haz the power, 2016-06-30 21:00:19]

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

[dogben, 2016-06-30 21:03:32]

lgtm

[ethannicholas, 2016-06-30 21:06:07]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2016-06-30 21:06:09]

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/1984363002/#ps600001
(title: "fixed linux build issue")

[commit-bot: I haz the power, 2016-06-30 21:06:20]

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-06-30 21:19:30]

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

[commit-bot: I haz the power, 2016-06-30 21:19:33]

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-01 13:57:15]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2016-07-01 13:57:16]

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/1984363002/#ps620001
(title: "fixed test error")

[commit-bot: I haz the power, 2016-07-01 13:57:25]

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-01 14:14:27]

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

[commit-bot: I haz the power, 2016-07-01 14:14:30]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[ethannicholas, 2016-07-01 14:46:20]

Description was changed from

==========
initial checkin of SkSL compiler

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
initial checkin of SkSL compiler

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia.compile:Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot,Build-Mac-Clang-x86_64-Release-CMake-Trybot
==========

[ethannicholas, 2016-07-01 14:47:50]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2016-07-01 14:47:52]

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/1984363002/#ps640001
(title: "fixed CMake build")

[commit-bot: I haz the power, 2016-07-01 14:48:01]

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-01 15:22:05]

Description was changed from

==========
initial checkin of SkSL compiler

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia.compile:Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot,Build-Mac-Clang-x86_64-Release-CMake-Trybot
==========

to

==========
initial checkin of SkSL compiler

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia.compile:Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot,Build-Mac-Clang-x86_64-Release-CMake-Trybot

Committed:
https://skia.googlesource.com/skia/+/b3058bdb1049ca75d526eb9f11e1a42a49e63585
==========

[commit-bot: I haz the power, 2016-07-01 15:22:08]

Committed patchset #33 (id:640001) as
https://skia.googlesource.com/skia/+/b3058bdb1049ca75d526eb9f11e1a42a49e63585

[dogben, 2016-07-01 15:28:55]

lgtm