added initial GLSL support to skslc

added initial GLSL support to skslc

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2185393003

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

[ethannicholas, 2016-07-28 15:23:00]

Description was changed from

==========
added initial GLSL support to skslc

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

to

==========
added initial GLSL support to skslc

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2185393003
==========

[ethannicholas, 2016-07-28 17:31:10]

Patchset #1 (id:1) has been deleted

[ethannicholas, 2016-07-28 17:35:05]

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

[dogben, 2016-07-31 23:20:21]

Would be nice to have unit tests.

As usual, anything that says "nit" is ignorable.

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

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLCompiler.h...
src/sksl/SkSLCompiler.h:40: bool toGLSL(Program::Kind kind, const std::string&
text, GLCaps caps, std::ostream& out);
nit: indentation doesn't seem to match above.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
File src/sksl/SkSLGLSLCodeGenerator.cpp (right):

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:22: if (fAtLineStart) {
nit: maybe "if (s[0] == '\0') return;"? Maybe I'm being too OCD, but it bugs me
that writeLine() will indent the empty line.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:65: this->writeLine(f.fType.name() + " " +
f.fName + ";");
Missing modifiers?

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:73: case Expression::kBinary_Kind:
nit: Indentation is inconsistent.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:95:
this->writePrefixExpression((PrefixExpression&) expr);
Don't we need to pass in parentPrecedence? Maybe it doesn't matter because all
postfix operations operate on things that no prefix operations can produce?

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:123: this->writeExpression(*arg,
kTopLevel_Precedence);
nit: kSequence_Precedence, although it shouldn't matter assuming SkSL will never
support the sequence operator.

Same for writeConstructor.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:248: this->write(to_string(f.fValue));
Docs for std::to_string say std::to_string(1e-9) == "0.000000", so I don't think
this will work.
http://en.cppreference.com/w/cpp/string/basic_string/to_string

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:257: this->write(param->fType.name() + " " +
param->fName);
Missing modifiers and sizes.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:272: this->writeStruct(intf.fVariable.fType);
We're generating a struct for an interface? I don't see how that will work for
anonymous interface fields.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:277: if
(decl.fVars[i]->fModifiers.fLayout.fBuiltin >= 0) {
I don't know what this is about. builtin isn't documented in the GLSL spec or
anywhere in src/sksl.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:281: this->write(decl.fVars[i]->fType.name()
+ " " + decl.fVars[i]->fName);
If this is a top-level var declaration, fType might need to be a struct
declaration.

Missing sizes.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:284: this->writeExpression(*decl.fValues[i],
kTopLevel_Precedence);
nit: kSequence_Precedence, with same caveat as above.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:356: }
else write semicolon

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
File src/sksl/SkSLGLSLCodeGenerator.h (right):

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.h:100: void write(std::string& s);
nit: seems like "void write(const std::string& s);" could take the place of both
std::string and std::string& overrides.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.h:106: void writeProgramElement(ProgramElement&
pe);
This method doesn't seem to be implemented or used.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.h:108: void writeExtension(Extension& ext);
nit: seems odd that some methods take const ref and others take non-const ref.
Looks the same as SkSLSPIRVCodeGenerator, though, so not a big deal.

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

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLIRGenerato...
src/sksl/SkSLIRGenerator.cpp:584: true));
nit: use enum, or "/*inAnonymousInterfaceBlock*/ true"

Same nit below.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLIRGenerato...
src/sksl/SkSLIRGenerator.cpp:666: *outLeftType = &left;
Seems like you need *outLeftType = &(*outResultType)->toCompound(context,
left.columns(), left.rows()); and similarly for outRightType. I assume
SpvOpMatrixTimesVector doesn't work for dmat3 * vec3.

Same below.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLIRGenerato...
src/sksl/SkSLIRGenerator.cpp:669: return left.rows() == right.columns();
Should be left.columns() == right.columns() because the vector is implicitly
transposed.

(Or maybe I'm confused, and it's implicitly transposed below, in which case you
would need left.columns() == right.columns() below.)

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/ir/SkSLLayout.h
File src/sksl/ir/SkSLLayout.h (left):

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/ir/SkSLLayout....
src/sksl/ir/SkSLLayout.h:74: const int fLocation;
nit: Why remove const?

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/ir/SkSLModifie...
File src/sksl/ir/SkSLModifiers.h (left):

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/ir/SkSLModifie...
src/sksl/ir/SkSLModifiers.h:76: const Layout fLayout;
nit: Why remove const?

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

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/ir/SkSLVariabl...
src/sksl/ir/SkSLVariable.h:43: mutable Modifiers fModifiers;
nit: don't need mutable

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/lex.sksl.c
File src/sksl/lex.sksl.c (left):

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/lex.sksl.c#old...
src/sksl/lex.sksl.c:7: 
I didn't review this file.

[dogben, 2016-08-01 13:01:46]

BTW, can we just disable arrays in the parser and not worry about them? Maybe
disable double also?

[egdaniel, 2016-08-01 13:18:28]

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
File src/sksl/SkSLGLSLCodeGenerator.cpp (right):

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:162: this->write(&("x\0y\0z\0w\0"[c * 2]));
not that functionally it really matters, but is the idea to just always use xyzw
even for colors that we wrote rbga in the shaders?

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:396: if (fCaps.fStandard ==
GLCaps::kGLES_Standard) {
I don't see fCaps used anywhere else so why split it into int and enum? Why not
just have a version string that includes the whole thing?

[ethannicholas, 2016-08-02 16:12:34]

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

[commit-bot: I haz the power, 2016-08-02 16:12:44]

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

[ethannicholas, 2016-08-02 16:13:18]

Added some unit tests (more will come over time, of course), fixed whitespace,
and hopefully addressed all of the other comments.

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

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLCompiler.h...
src/sksl/SkSLCompiler.h:40: bool toGLSL(Program::Kind kind, const std::string&
text, GLCaps caps, std::ostream& out);
On 2016/07/31 23:20:19, Ben Wagner wrote:
> nit: indentation doesn't seem to match above.

Evidently one of the editors I use was not properly configured for spaces vs.
tabs, and some tabs snuck in. I have replaced all of them with spaces.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
File src/sksl/SkSLGLSLCodeGenerator.cpp (right):

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:22: if (fAtLineStart) {
On 2016/07/31 23:20:20, Ben Wagner wrote:
> nit: maybe "if (s[0] == '\0') return;"? Maybe I'm being too OCD, but it bugs
me
> that writeLine() will indent the empty line.

Done.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:65: this->writeLine(f.fType.name() + " " +
f.fName + ";");
On 2016/07/31 23:20:20, Ben Wagner wrote:
> Missing modifiers?

Fixed.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:73: case Expression::kBinary_Kind:
On 2016/07/31 23:20:20, Ben Wagner wrote:
> nit: Indentation is inconsistent.

Damn you, tabs!

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:95:
this->writePrefixExpression((PrefixExpression&) expr);
On 2016/07/31 23:20:20, Ben Wagner wrote:
> Don't we need to pass in parentPrecedence? Maybe it doesn't matter because all
> postfix operations operate on things that no prefix operations can produce?

We sure do!

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:162: this->write(&("x\0y\0z\0w\0"[c * 2]));
On 2016/08/01 13:18:27, egdaniel wrote:
> not that functionally it really matters, but is the idea to just always use
xyzw
> even for colors that we wrote rbga in the shaders?

Correct. Since it doesn't matter whether you use rgba / xyzw / stpq when
swizzling, the compiler doesn't actually keep track of which ones you picked.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:248: this->write(to_string(f.fValue));
On 2016/07/31 23:20:19, Ben Wagner wrote:
> Docs for std::to_string say std::to_string(1e-9) == "0.000000", so I don't
think
> this will work.
> http://en.cppreference.com/w/cpp/string/basic_string/to_string

My implementation of to_string (note that this is not std::to_string) now always
uses sufficient digits.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:257: this->write(param->fType.name() + " " +
param->fName);
On 2016/07/31 23:20:19, Ben Wagner wrote:
> Missing modifiers and sizes.

Fixed.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:272: this->writeStruct(intf.fVariable.fType);
On 2016/07/31 23:20:20, Ben Wagner wrote:
> We're generating a struct for an interface? I don't see how that will work for
> anonymous interface fields.

Stupid extra "struct" keyword. Fixed.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:277: if
(decl.fVars[i]->fModifiers.fLayout.fBuiltin >= 0) {
On 2016/07/31 23:20:20, Ben Wagner wrote:
> I don't know what this is about. builtin isn't documented in the GLSL spec or
> anywhere in src/sksl.

It's from the SPIR-V spec. I've added a comment to that effect.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:281: this->write(decl.fVars[i]->fType.name()
+ " " + decl.fVars[i]->fName);
On 2016/07/31 23:20:20, Ben Wagner wrote:
> If this is a top-level var declaration, fType might need to be a struct
> declaration.
> 
> Missing sizes.

Done.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:284: this->writeExpression(*decl.fValues[i],
kTopLevel_Precedence);
On 2016/07/31 23:20:20, Ben Wagner wrote:
> nit: kSequence_Precedence, with same caveat as above.

Done.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:356: }
On 2016/07/31 23:20:20, Ben Wagner wrote:
> else write semicolon

Done.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:396: if (fCaps.fStandard ==
GLCaps::kGLES_Standard) {
On 2016/08/01 13:18:27, egdaniel wrote:
> I don't see fCaps used anywhere else so why split it into int and enum? Why
not
> just have a version string that includes the whole thing?

Going to start paying a lot more attention to it in the next change. Right now
this is just "this basically produces correct GLSL". I have to go from that to
"this produces correct GLSL / GLSL ES for every single device we support", and
for that I need to know the version and standard.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
File src/sksl/SkSLGLSLCodeGenerator.h (right):

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.h:100: void write(std::string& s);
On 2016/07/31 23:20:20, Ben Wagner wrote:
> nit: seems like "void write(const std::string& s);" could take the place of
both
> std::string and std::string& overrides.

Fixed.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.h:106: void writeProgramElement(ProgramElement&
pe);
On 2016/07/31 23:20:20, Ben Wagner wrote:
> This method doesn't seem to be implemented or used.

Fixed.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.h:108: void writeExtension(Extension& ext);
On 2016/07/31 23:20:20, Ben Wagner wrote:
> nit: seems odd that some methods take const ref and others take non-const ref.
> Looks the same as SkSLSPIRVCodeGenerator, though, so not a big deal.

Fixed.

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

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLIRGenerato...
src/sksl/SkSLIRGenerator.cpp:584: true));
On 2016/07/31 23:20:20, Ben Wagner wrote:
> nit: use enum, or "/*inAnonymousInterfaceBlock*/ true"
> 
> Same nit below.

Done.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/SkSLIRGenerato...
src/sksl/SkSLIRGenerator.cpp:666: *outLeftType = &left;
On 2016/07/31 23:20:21, Ben Wagner wrote:
> Seems like you need *outLeftType = &(*outResultType)->toCompound(context,
> left.columns(), left.rows()); and similarly for outRightType. I assume
> SpvOpMatrixTimesVector doesn't work for dmat3 * vec3.
> 
> Same below.

Completely reworked this.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/ir/SkSLLayout.h
File src/sksl/ir/SkSLLayout.h (left):

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/ir/SkSLLayout....
src/sksl/ir/SkSLLayout.h:74: const int fLocation;
On 2016/07/31 23:20:21, Ben Wagner wrote:
> nit: Why remove const?

Because I added support for redeclaring a builtin variable, which has the effect
of updating the modifiers attached to it, so the modifiers need to be
modifiable.

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/ir/SkSLModifie...
File src/sksl/ir/SkSLModifiers.h (left):

https://codereview.chromium.org/2185393003/diff/20001/src/sksl/ir/SkSLModifie...
src/sksl/ir/SkSLModifiers.h:76: const Layout fLayout;
On 2016/07/31 23:20:21, Ben Wagner wrote:
> nit: Why remove const?

See my response about Layout.

[commit-bot: I haz the power, 2016-08-02 16:23:34]

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

[commit-bot: I haz the power, 2016-08-02 16:23:35]

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

[dogben, 2016-08-02 18:08:16]

https://codereview.chromium.org/2185393003/diff/40001/src/sksl/SkSLGLSLCodeGe...
File src/sksl/SkSLGLSLCodeGenerator.cpp (right):

https://codereview.chromium.org/2185393003/diff/40001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:62: this->writeType(f.fType);
Example from the GLSL spec:
struct S { float f; };
struct T {
    S; // Error: anonymous structures disallowed
    struct { ... }; // Error: embedded structures disallowed
    S s; // Okay: nested structures with name are allowed
};

If someone writes #3 (nested structures with name are allowed) in SkSL, you will
output #2 (embedded structures disallowed) as GLSL.

I think you may run into a similar problem with variable declarations, e.g.:
struct S { float f; };
S first_var;
S second_var;

would become:
struct S { float f; } first_var;
struct S { float f; } second_var;

which would cause a redefinition error.

https://codereview.chromium.org/2185393003/diff/40001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:68: this->write(type.name());
Maybe add a comment that array sizes are part of the type name.

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

https://codereview.chromium.org/2185393003/diff/40001/src/sksl/SkSLIRGenerato...
src/sksl/SkSLIRGenerator.cpp:685: rightColumns = right.rows();
nit: Maybe add comment: "Vector on right is implicitly transposed to
single-column."

https://codereview.chromium.org/2185393003/diff/40001/src/sksl/SkSLIRGenerato...
src/sksl/SkSLIRGenerator.cpp:692: if (rightColumns > 1) {
nit: Maybe add comment: "Single-column result is implicitly transposed to
vector."

[ethannicholas, 2016-08-02 20:42:27]

Patchset #4 (id:80001) has been deleted

[ethannicholas, 2016-08-02 20:43:06]

https://codereview.chromium.org/2185393003/diff/40001/src/sksl/SkSLGLSLCodeGe...
File src/sksl/SkSLGLSLCodeGenerator.cpp (right):

https://codereview.chromium.org/2185393003/diff/40001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:62: this->writeType(f.fType);
On 2016/08/02 18:08:15, Ben Wagner wrote:
> Example from the GLSL spec:
> struct S { float f; };
> struct T {
>     S; // Error: anonymous structures disallowed
>     struct { ... }; // Error: embedded structures disallowed
>     S s; // Okay: nested structures with name are allowed
> };
> 
> If someone writes #3 (nested structures with name are allowed) in SkSL, you
will
> output #2 (embedded structures disallowed) as GLSL.
> 
> I think you may run into a similar problem with variable declarations, e.g.:
> struct S { float f; };
> S first_var;
> S second_var;
> 
> would become:
> struct S { float f; } first_var;
> struct S { float f; } second_var;
> 
> which would cause a redefinition error.

Good catch, fixed.

https://codereview.chromium.org/2185393003/diff/40001/src/sksl/SkSLGLSLCodeGe...
src/sksl/SkSLGLSLCodeGenerator.cpp:68: this->write(type.name());
On 2016/08/02 18:08:15, Ben Wagner wrote:
> Maybe add a comment that array sizes are part of the type name.

Done.

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

https://codereview.chromium.org/2185393003/diff/40001/src/sksl/SkSLIRGenerato...
src/sksl/SkSLIRGenerator.cpp:685: rightColumns = right.rows();
On 2016/08/02 18:08:15, Ben Wagner wrote:
> nit: Maybe add comment: "Vector on right is implicitly transposed to
> single-column."

Done.

https://codereview.chromium.org/2185393003/diff/40001/src/sksl/SkSLIRGenerato...
src/sksl/SkSLIRGenerator.cpp:692: if (rightColumns > 1) {
On 2016/08/02 18:08:15, Ben Wagner wrote:
> nit: Maybe add comment: "Single-column result is implicitly transposed to
> vector."

Done.

[dogben, 2016-08-02 21:19:23]

lgtm

[ethannicholas, 2016-08-03 18:30:57]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2016-08-03 18:30:58]

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/2185393003/#ps120001
(title: "fixed Windows build for real")

[commit-bot: I haz the power, 2016-08-03 18:31:06]

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-08-03 18:37:55]

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

[commit-bot: I haz the power, 2016-08-03 18:37:56]

Try jobs failed on following builders:
  Build-Ubuntu-Clang-x86_64-Debug-GN-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)

[ethannicholas, 2016-08-03 19:19:52]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2016-08-03 19:19: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/2185393003/#ps140001
(title: "fixed gn build")

[commit-bot: I haz the power, 2016-08-03 19:19:56]

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-08-03 19:43:38]

Description was changed from

==========
added initial GLSL support to skslc

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2185393003
==========

to

==========
added initial GLSL support to skslc

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2185393003

Committed:
https://skia.googlesource.com/skia/+/f789b3893579b773bb4d7be6c2c65311500b53bb
==========

[commit-bot: I haz the power, 2016-08-03 19:43:39]

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