added constant folding & branch elimination to skslc

added constant folding & branch elimination to skslc

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

Committed: https://skia.googlesource.com/skia/+/6136310ee8f43247548bcefcaeca6d43023c10aa
Committed: https://skia.googlesource.com/skia/+/08a9211a8492a84e1f4a6899759f8f37ed5aec3e

[ethannicholas, 2016-11-08 20:26:28]

Description was changed from

==========
added constant folding & branch elimination to skslc

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

to

==========
added constant folding & branch elimination to skslc

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

[ethannicholas, 2016-11-08 20:54:31]

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

[commit-bot: I haz the power, 2016-11-08 20:54:38]

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

[ethannicholas, 2016-11-08 21:41:33]

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

[ethannicholas, 2016-11-08 21:41:33]

Added constant folding & branch elimination to skslc.

I don't expect this to have any immediate impact -- the GL and Vulkan backends
are presumably both performing these optimization as well, so our doing so
shouldn't change anything -- but this is in preparation for exposing GL caps
within sksl. The intent is to have sksl programs be able to check whether a
particular feature is available using an ordinary 'if' statement, with the other
branch (potentially containing code that would cause errors in GLSL due to using
unsupported features) being eliminated statically.

[bsalomon, 2016-11-08 22:17:10]

To the extent I understand this, lgtm.

[dogben, 2016-11-09 15:35:36]

lgtm

Code LGTM. It would be great to document this feature of SkSL and any caveats
(e.g. how overflow is handled).

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

https://codereview.chromium.org/2489673002/diff/20001/src/sksl/SkSLIRGenerato...
src/sksl/SkSLIRGenerator.cpp:831: int64_t leftVal  = ((IntLiteral&)
left).fValue;
Assuming integer overflow is well-defined in SkSL, this seems a bit dicey. SkSL
(and GLSL/SpirV) ints will be 32 bits (right?), but we are calculating the
result using int64_t. I could probably convince myself this is OK if we 
 - cast the result to int32_t or uint32_t, and
 - either 
   - ensure that anything that goes into IntLiteral is 32 bits, or
   - cast leftVal and rightVal to int32_t or uint32_t.

(The second part is probably not strictly necessary for all operators, but it
doesn't seem worthwhile to make special cases for those.)

Even if integer overflow produces undefined results, I would still expect SHR
and SHL to be well-defined.

https://codereview.chromium.org/2489673002/diff/20001/src/sksl/SkSLIRGenerato...
src/sksl/SkSLIRGenerator.cpp:855: double leftVal  = ((FloatLiteral&)
left).fValue;
Same as above, except I don't feel that strongly about it, because if we're
relying on floating point precision to get the correct result, we are doing
something wrong.

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

https://codereview.chromium.org/2489673002/diff/20001/src/sksl/SkSLIRGenerato...
src/sksl/SkSLIRGenerator.h:92: std::unique_ptr<Expression> constantFold(const
Expression& left,
nit: Should probably document that returning nullptr does not indicate a
compilation error.

[ethannicholas, 2016-11-09 16:36:22]

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

https://codereview.chromium.org/2489673002/diff/20001/src/sksl/SkSLIRGenerato...
src/sksl/SkSLIRGenerator.cpp:831: int64_t leftVal  = ((IntLiteral&)
left).fValue;
On 2016/11/09 15:35:36, dogben wrote:
> Assuming integer overflow is well-defined in SkSL, this seems a bit dicey.
SkSL
> (and GLSL/SpirV) ints will be 32 bits (right?), but we are calculating the
> result using int64_t. I could probably convince myself this is OK if we 
>  - cast the result to int32_t or uint32_t, and
>  - either 
>    - ensure that anything that goes into IntLiteral is 32 bits, or
>    - cast leftVal and rightVal to int32_t or uint32_t.
> 
> (The second part is probably not strictly necessary for all operators, but it
> doesn't seem worthwhile to make special cases for those.)
> 
> Even if integer overflow produces undefined results, I would still expect SHR
> and SHL to be well-defined.

In GLSL ES, integer precision varies. It is allowed to be as low as 10 bits +
sign bit, and integer overflow is undefined. Furthermore, we do cast the result
to (u)int32 when generating code.

It's possible to detect the precision using GL functions and error appropriately
at compile time, but I don't have access to that from within SkSL yet. The plan
is for my next change to move all of the Skia GLSL caps into SkSL's hands, which
would allow me to perform that check. Given that this CL does not introduce new
problems (you can easily write undefined overflowing expressions in GLSL today),
I'll push as-is and revisit when I have the information I need to fix this.

I've added a comment to clarify this.

https://codereview.chromium.org/2489673002/diff/20001/src/sksl/SkSLIRGenerato...
src/sksl/SkSLIRGenerator.cpp:855: double leftVal  = ((FloatLiteral&)
left).fValue;
On 2016/11/09 15:35:36, dogben wrote:
> Same as above, except I don't feel that strongly about it, because if we're
> relying on floating point precision to get the correct result, we are doing
> something wrong.

Floating point's range is allowed to be as small as -2 ... +2 (no, that is not a
typo) but could be much larger. For right now, I'll assume the author isn't
statically writing expressions that won't actually work, and as above look into
detecting this properly when I have access to all of the caps.

[ethannicholas, 2016-11-09 16:37:19]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2016-11-09 16:37:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjaminwagner@google.com, bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/2489673002/#ps40001
(title: "added some clarifying comments")

[commit-bot: I haz the power, 2016-11-09 16:37:29]

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-11-09 17:09:28]

Description was changed from

==========
added constant folding & branch elimination to skslc

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

to

==========
added constant folding & branch elimination to skslc

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

Committed:
https://skia.googlesource.com/skia/+/6136310ee8f43247548bcefcaeca6d43023c10aa
==========

[commit-bot: I haz the power, 2016-11-09 17:09:29]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/6136310ee8f43247548bcefcaeca6d43023c10aa

[scroggo, 2016-11-09 20:03:05]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2494523002/ by scroggo@google.com.

The reason for reverting is: Breaking the DEPS roll:

../../third_party/skia/src/sksl/SkSLIRGenerator.cpp:257:70: error: chosen
constructor is explicit in copy-initialization
            return std::unique_ptr<Statement>(new Block(s.fPosition, { },
fSymbolTable));
                                                                     ^~~
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/debug/vector:77:7:
note: constructor declared here
      vector(const _Allocator& __a = _Allocator())
      ^
../../third_party/skia/src/sksl/ir/SkSLBlock.h:20:70: note: passing argument to
parameter 'statements' here
    Block(Position position, std::vector<std::unique_ptr<Statement>> statements,
                                                                     ^
.

[ethannicholas, 2016-11-09 20:44:24]

Description was changed from

==========
added constant folding & branch elimination to skslc

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

Committed:
https://skia.googlesource.com/skia/+/6136310ee8f43247548bcefcaeca6d43023c10aa
==========

to

==========
added constant folding & branch elimination to skslc

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

Committed:
https://skia.googlesource.com/skia/+/6136310ee8f43247548bcefcaeca6d43023c10aa
==========

[ethannicholas, 2016-11-09 20:52:48]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2016-11-09 20:52:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjaminwagner@google.com, bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/2489673002/#ps60001
(title: "fixed compile error")

[commit-bot: I haz the power, 2016-11-09 20:52:53]

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

[commit-bot: I haz the power, 2016-11-09 21:26:47]

Description was changed from

==========
added constant folding & branch elimination to skslc

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

Committed:
https://skia.googlesource.com/skia/+/6136310ee8f43247548bcefcaeca6d43023c10aa
==========

to

==========
added constant folding & branch elimination to skslc

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

Committed:
https://skia.googlesource.com/skia/+/6136310ee8f43247548bcefcaeca6d43023c10aa
Committed:
https://skia.googlesource.com/skia/+/08a9211a8492a84e1f4a6899759f8f37ed5aec3e
==========

[commit-bot: I haz the power, 2016-11-09 21:26:48]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/08a9211a8492a84e1f4a6899759f8f37ed5aec3e