SkSL internally tracks token offsets, but only ever reports errors using
line numbers. With the introduction of the DSL, which (being embedded in
C++ source) only has access to line numbers in the first place, tracking
offsets went from merely providing little benefit to actively making
life more difficult.
We are changing SkSL's position tracking from handling offsets to
handling line numbers, but to simplify the review process the change is
split up into two main steps. The first step (this CL) starts using
line numbers everywhere, but avoids the thousand-line churn of actually
renaming "offset", so most "offset" fields, variables, and parameters
will be briefly misnamed and will actually contain a line number.
The followup CL will complete the process by renaming all of the
now-misnamed fields, variables, and parameters, but will not make any
behavioral changes.
Bug: skia:12459
Change-Id: I30dc87cf4b816c5ddd7b8ae1be32586388962085
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/451419
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
It looks like returning from inside a switch on iOS gives wrong results
in GLSL.
Change-Id: I9d6d8971a7a54600268e27443815444fca6f3c61
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450994
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This fails on several platforms in practice, and is of very limited
real-world utility.
Change-Id: Ib476396fc33cb51af6bbcf7fe822d30703ed995d
Bug: skia:12467
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450993
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I17b5e21a28140b8e9313d87af9b1145674214fdb
Bug: skia:12450
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450989
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Also, removed "switch containing dead code" test. This wasn't testing
anything meaningful. (When we had full CFG analysis, we could have
eliminated some of the assignments inside the switch body, but this is
not something we do anymore.)
Change-Id: Iaeb74ebee41a7f368113ede9a4e30c033b9de8ac
Bug: skia:12450
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450985
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
The Mac Radeon GLSL driver crashes when given a switch statement that
only contains a default case and returns a value. Adding a case works
around the crash, and doesn't affect the meaning of the switch.
Change-Id: Iabbd267e0e31e8df7d3b7e747a7204d50931d0be
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450977
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This is a reland of be056f4f62
The Switch test has been restructured to dodge an iOS bug.
Original change's description:
> Add switch statement support to PipelineStage.
>
> This allows us to write SKSL_TEST_ES3 tests in SkSLTest and have them
> run properly. Previously, such a test would assert inside the pipeline-
> stage generator. In ES2 mode, we will rewrite switches as chained ifs,
> but in ES3 mode we will want to continue emitting them as-is (they will
> be faster than chained ifs on a modern GPU).
>
> `writeSwitchStatement` is adapted from GLSLCodeGenerator.
>
> Change-Id: I532ea5ed49869e7cdffced0cdcd0e353af8d4d79
> Bug: skia:12450
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450478
> Commit-Queue: John Stiles <johnstiles@google.com>
> Commit-Queue: Brian Osman <brianosman@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
Bug: skia:12450
Change-Id: I5102081c636ef09cd23f5bc894e6c96e92a4c121
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450757
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This fixes a driver bug with the Nexus 7 while retaining the meaningful
part of the test.
Change-Id: I98edab32132f0c52a1f69b03efd403fae43c336b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450482
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit be056f4f62.
Reason for revert: apparently switch on iOS GLSL is extremely broken
Original change's description:
> Add switch statement support to PipelineStage.
>
> This allows us to write SKSL_TEST_ES3 tests in SkSLTest and have them
> run properly. Previously, such a test would assert inside the pipeline-
> stage generator. In ES2 mode, we will rewrite switches as chained ifs,
> but in ES3 mode we will want to continue emitting them as-is (they will
> be faster than chained ifs on a modern GPU).
>
> `writeSwitchStatement` is adapted from GLSLCodeGenerator.
>
> Change-Id: I532ea5ed49869e7cdffced0cdcd0e353af8d4d79
> Bug: skia:12450
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450478
> Commit-Queue: John Stiles <johnstiles@google.com>
> Commit-Queue: Brian Osman <brianosman@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
Bug: skia:12450
Change-Id: If40c90023a64c608181285f6470b3e75303cc3cc
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450756
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
This allows us to write SKSL_TEST_ES3 tests in SkSLTest and have them
run properly. Previously, such a test would assert inside the pipeline-
stage generator. In ES2 mode, we will rewrite switches as chained ifs,
but in ES3 mode we will want to continue emitting them as-is (they will
be faster than chained ifs on a modern GPU).
`writeSwitchStatement` is adapted from GLSLCodeGenerator.
Change-Id: I532ea5ed49869e7cdffced0cdcd0e353af8d4d79
Bug: skia:12450
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450478
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
We didn't have a test case for this particular construct, but we will
emit special code to handle it when rewriting switch statements.
Change-Id: I7ac632f7bee348194940812c956c8a7df51ffaff
Bug: skia:12450
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450477
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Unreachable code might contain the only reference to a variable or
function. We can eliminate those variables/functions if we remove the
unreachable code first.
(Are there counterexamples where this order leads to worse results? I
couldn't think of any, and pragmatically it didn't show up in any of
our existing tests.)
Change-Id: Ic9f0222851269e0c37eb9570547307998f882b6c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450156
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
`increment` and `float a` could be eliminated, but are not.
This is fixed in a followup CL.
Change-Id: I7a5c3ab7341f40020f84f157b08a7152bc067af0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450276
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
The GLSL spec allows matrix constructors containing vectors that would
split between multiple columns of the matrix. However, in practice, this
does not actually work well on a lot of GPUs!
- "cast not allowed", "internal error":
Tegra 3
Quadro P400
GTX 660
GTX 960
- Compiles, but generates wrong result:
RadeonR9M470X
RadeonHD7770
Since this isn't a pattern we expect to see in user code, we now report
it as an error at compile time. mat2(vec4) is treated as an exceptional
case and still allowed.
Change-Id: Id6925984a2d1ec948aec4defcc790a197a96cf86
Bug: skia:12443
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/449518
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Not a big deal necessarily, but considering using this logic in GLSL as
well, and I'm less confident that your average GLSL ES driver will
optimize away the separate array loads.
Change-Id: I6a9f0d18c0fac138f64ad6426670f615e17f3492
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/449099
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Adjusted default caps in skslc to be consistent with runtime behavior,
and added optional settings mode to enable the feature. Tests for both
scenarios. (The error test crashed prior to the fix).
Bug: oss-fuzz:38726
Change-Id: I5270d4837ac982085d7baf5abd4b361f7bfb8562
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/449062
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
We didn't have any tests which exercised the non-square matrix case
(because such a test requires ES3), so it was silently broken. It's
now fixed. The tests exposed a DIFFERENT Quadro P400 bug which will be
fixed separately.
Change-Id: Icf24acad5ea6f18aea3d8aa5a903e7bea41a5c23
Bug: skia:12443
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/448379
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This exposes a bug in the Metal code generator which will be resolved
in a followup CL.
Change-Id: If073835dbee474ea9a805eb92b42dc1fca2afbd0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/448378
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This reverts commit 0f4304e6e7.
Reason for revert: breaks Adreno 6xx
Original change's description:
> Add RelaxedPrecision decoration to function-call temp vars.
>
> This is really same basic issue as http://review.skia.org/446640. We
> were creating a temp variable but ignoring its type's precision.
>
> Change-Id: I9a5fedd7ada864d36757fc196f42ff95bac7d706
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/446718
> Commit-Queue: John Stiles <johnstiles@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I6ae4e264b60f7f38a1abb5f1d0324461a33c896d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/446742
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
This is really same basic issue as http://review.skia.org/446640. We
were creating a temp variable but ignoring its type's precision.
Change-Id: I9a5fedd7ada864d36757fc196f42ff95bac7d706
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/446718
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
We don't need to initialize input values for `out` params. (We were
treating them the same as `inout`.)
Change-Id: Ib447d15de237a6a03740ad012180691dc60a50bd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/446717
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously we were not honoring the variable precision at all (passing
null to nextId).
Change-Id: I0e217e6c0d3d701dc0540d4d4069a3597abdad11
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/446640
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Several fuzzer issues, and one Chromium issue that's blocking the roll.
Bug: chromium:1246795
Bug: skia:12423
Change-Id: I00370b74569b447e543d9a1f22c588eb493063da
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/445960
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: Brian Osman <brianosman@google.com>
Change-Id: Id894eb70273454716eb33c85dff2056333e90cdd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/445281
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
The fuzzer discovered that, when we attempt to verify that an array
doesn't contain any literal values that are out-of-range for its base
type, we pay a linear-time cost based on the size of the array. This
happens even when the array value isn't known at compile time; we still
iterate over its slot count and diligently discover that every single
constant-subexpression slot in the expression is "null".
We now have a helper function on Expression,
`allowsConstantSubexpressions`, which only returns true for expression
kinds that can contain constant subexpressions. We use this helper to
skip over this linear-per-subexpression check when the expression
cannot possibly contain a constant subexpression. In particular,
`AnyConstructor::compareConstant` and `Type::checkForOutOfRangeLiteral`
will now early-out for expressions that can't possibly contain a
constant subexpression.
Change-Id: Ia34e422afa67b478a8616acb0a0e9cd211b29698
Bug: oss-fuzz:37900
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/444136
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
We had a logic bug when attempting to optimize the following code:
const vecN x = vecN(a, b, c);
-x;
The goal was to replace `-x` with `vecN(-a, -b, -c)` but we accidentally
tried to cast the `x` VariableReference to a Constructor. We
unfortunately didn't cover this in any of our test cases, but the fuzzer
managed to synthesize it by mixing and matching elements from its new
corpus.
This affected several different constructor types: splat, diagonal-
matrix, compound and array.
Change-Id: I10dd2460ab26ba3e820b0cff5db091368fb7e648
Bug: oss-fuzz:37764, oss-fuzz:37861
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/443407
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Bug: skia:8451 skia:10827
Change-Id: I5b38a1d72cd4558f8e2a92aaf9b12f05efce0923
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/442683
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Chris Dalton <csmartdalton@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This is a first step towards replacing `finalizeFunction` with a
`FunctionDefinition::Convert` method living outside of the IRGenerator.
Previously this code would assert that we had no early returns from a
vertex-program main() method; this has been turned into an error.
(The original assertion was also tied to fRTFlip, because the *problem*
with early-returns in main is tied to the lack of RTFlip fixups, but
we fundamentally don't allow early returns, so it makes more sense to
just universally disallow it.)
Change-Id: Iba0742f7ef3cbc83995ea130fec1eb1ef2556c44
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/442691
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
No-op arithmetic simplification will convert expressions like `x += 0`
to `x`. When making this simplification, we will also downgrade the ref-
kind of `x` from "write" to "read" since the new expression is no longer
an assignment.
The fuzzer discovered that the ref-kind downgrade was too aggressive,
and would also traverse into nested subexpressions and downgrade them
as well. That is, for `x[y=z] += 0` would convert both `x` and `y`
into "read" references, which is incorrect; `y` is still being written
to.
The fuzzer managed to turn this mistake into an assertion by leveraging
a separate optimization. It added a leading, side-effect-less comma
expression for us to detect as worthless and eliminate. In doing so, we
clone the expression with the busted ref-kind, triggering an assertion.
Change-Id: I42fc31f6932f679ae875e2b49db2ad2f4e89e2cb
Bug: oss-fuzz:37677
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/442536
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Previously, a struct containing a vardecl with multiple declarations
would interpret arrays incorrectly. An array would be applied to ALL
variables in the decl after its initial appearance. That is,
`int w, x[10], y, z;` would be interpreted as
`int w, x[10], y[10], z[10];`.
This is now fixed and our test case runs as expected.
Change-Id: I5b4a617c58cdfb83face651effd42770a1f68638
Bug: oss-fuzz:37622
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/441879
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
The fuzzer detected a serious parsing error; a struct containing a
vardecl with multiple declarations would interpret arrays incorrectly.
An array would be applied to ALL variables in the decl after its initial
appearance. That is, `int w, x[10], y, z;` would be interpreted as
`int w, x[10], y[10], z[10];`. The fuzzer caught this by putting two
arrayed variables in a row; the second variable was interpreted as a
nested array, which led to an assertion.
This CL contains a simple hand-written test case demonstrating the bug,
with the fix coming in a followup.
Change-Id: I42d7372ba77fa1528ae24eb8c29a2e5903784139
Bug: oss-fuzz:37622
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/441878
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I26754745aa26313a2f76a86bd41699c7ac5b8a46
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/441596
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
The additional tests from http://review.skia.org/441238 uncovered a gap
in the constant folder's abilities; it was not able to fold away
boolean vector comparisons even when they were constant. These are ES2
constant-expressions, so folding them properly is a requirement.
Change-Id: Ia0b4d5d1215c5fc2b247ac3f0dec4c8747d2153e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/441579
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
The inliner contained a type error when attempting to inline a function
that takes an array as input. The scratch copy of the array was created
as `float[123] var;` instead of `float var[123];`. This led to an
assertion in VarDeclaration::Make.
Change-Id: I5128fe71462bb59a015a7b4e59c1a74800828b16
Bug: oss-fuzz:37466
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/441576
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
During constant-folding, we baked in an assertion stating that any
const-typed variable reference ought to have an initial value, because
you can't declare a const variable without assigning a value. However,
function parameters are an exception to this rule! They are variable
references and are allowed to be const, but will not have an initial
value. (In this case, `const` just means you can't alter the value.)
In this case, all we needed to do was remove the assertion; we already
treated this case defensively and with the appropriate care.
Change-Id: I61242c6d08c59886c6992898f195771e6334f2b4
Bug: oss-fuzz:37465
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/441239
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This was another place where we needed to use
`getConstantSubexpression` to rebuild vectors/matrices; it is a more
robust approach than trying handle each ctor type individually. The
fuzzer found an edge case with double-casting matrices to vectors that
fell through the cracks with the original approach.
In adding additional tests, I also found a case that the constant-folder
seems to ignore, `bool4(x,x,x,x) == bool4(x)`. This does fold for ints
and floats, so this ought to be fixable in a followup, but it's not a
big deal either way; this is very unlikely to occer in real code.
Change-Id: I4d577c87ef7049306685ca95250ecdf93b1dbc06
Bug: oss-fuzz:37464
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/441238
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Right now, Metal forces types to full precision. The matrix helper
functions previously baked in that assumption by hard-coding "floatX".
Now, they honor the component type; if this->typeName() started
returning "half", our helper functions would be named with "halfX". This
would allow half-precision and full-precision helpers to coexist.
Change-Id: I1679e6e76d2cf3c27fd69c42a92fb24bff6b69ec
Bug: skia:12339
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/439396
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
The value of 32 was causing errors when other uniforms were present, as
the SPIR-V code generator would detect overlapping uniform offsets and
fail.
Change-Id: I7bb1cf1244e54c39596c3a39e9f6972c6a47899c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/439059
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
An assignment like `mediump int a[2] = myHighpIntArray;` should succeed
now that the previous CLs have landed; originally, this would have
caused a type-mismatch error.
Change-Id: I86ffe6a21d0c7fbe289eef95aebc2605412566aa
Bug: skia:12248
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/437740
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Compiling a program with "allow narrowing conversions" actually fixes up
narrowing casts in the program by inserting casts wherever they would be
needed for type-correctness. For instance, compiling the statement
`half h = myFloat;`
inserts an appropriate narrowing cast:
`half h = half(myFloat);`.
The Pipeline stage code generator relies on this behavior, as when it
re-emits a runtime effect into a complete SkSL program, the narrowing-
conversions flag will no longer be set, but that is okay, because the
emitted code now contains typecasts anywhere they would be necessary.
Logically, this implies that anything which supports narrowing
conversions must be castable between high and low precision. In GLSL and
SPIR-V, such a cast is trivial, because the types are the same and the
precision qualifiers are treated as individual hints on each variable.
In Metal, we dodge the issue by only emitting full-precision types. But
we also need to emit raw SkSL from an SkSL program (that is what the
Pipeline stage generator does).
SkSL already supported every typical cast, but GLSL lacked any syntax
for casting an array to a different type. This meant SkSL had no array
casting syntax as well. SkSL now has array-cast syntax, but it is only
allowed for casting low/high-precision arrays to the same base type.
(You can't cast an int array to float, or a signed array to unsigned.)
Change-Id: Ia20933541c3bd4a946c1ea38209f93008acdb9cb
Bug: skia:12248
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/437687
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This is a reland of 23d8f94535
Original change's description:
> Fix array-of-matrix/struct comparisons in Metal.
>
> Metal needs helper functions in order to compare arrays, structs, and
> matrices. Depending on the input code, it was possible for the
> array-comparison helper to be emitted before a matrix-comparison
> or struct-comparison helper. If this occurred, array comparisons of that
> matrix or struct type would fail, because the operator== for the array's
> inner type was defined after array==, and Metal (like C++) parses
> top-to-bottom and only considers functions declared above the current
> function.
>
> We now emit prototypes for all the array, struct and matrix helper
> function. These prototypes are emitted above any helper functions. This
> ensures visibility no matter how your comparisons are organized.
>
> Change-Id: Ib3d8828c301fd0fa6c209788f9ea60800371edbe
> Bug: skia:12326
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/437739
> Commit-Queue: John Stiles <johnstiles@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
Bug: skia:12326
Change-Id: Ife68020f6b01fae973b97f76099c6d5e8215636c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/438296
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit ef9a1b66d0.
Reason for revert: not broken after all
Original change's description:
> Revert "Fix array-of-vector comparisons in Metal."
>
> This reverts commit 130338c9e1.
>
> Reason for revert: SkSL_ArrayComparison test causes Adreno 630/640 to crash in Vulkan
>
> Original change's description:
> > Fix array-of-vector comparisons in Metal.
> >
> > Comparing `vec1 == vec2` returns a bvec in Metal, so the result must be
> > wrapped in `all()` in order to boil it down to a single boolean result.
> > Our array-comparison helper function did not do this. Fortunately,
> > `all(scalar)` is a no-op, so we can just wrap the result unilaterally.
> >
> > Change-Id: I4f1f09a6832164ae2e6577d53b317f561332d581
> > Bug: skia:12324
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/437736
> > Auto-Submit: John Stiles <johnstiles@google.com>
> > Commit-Queue: Brian Osman <brianosman@google.com>
> > Reviewed-by: Brian Osman <brianosman@google.com>
>
> TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com,skcq-be@skia-corp.google.com.iam.gserviceaccount.com
>
> Change-Id: Ic76a5527a8339c8201f52df08d43041d7dcbeb61
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: skia:12324
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/438077
> Reviewed-by: John Stiles <johnstiles@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
# Not skipping CQ checks because this is a reland.
Bug: skia:12324
Change-Id: I3da699b8d1113800efb27e162d0c6315f0aeaa49
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/438176
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
The Metal code generator has historically avoided low-precision types in
the final output in order to dodge a variety of type-coercion issues.
However, the workaround was only coded for half/float. Extended the
workaround to cover int/short and uint/ushort as well.
Change-Id: I16e3a387ba2baef1ef2de7742e1b0d27786fee0e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/437688
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Previously, SPIR-V would generate two separate SpvIds for array types if
they differed in SkSL, even if they matched in SPIR-V. For instance,
`half[10]` and `float[10]` are the same SPIR-V type, so they should
reuse the same SpvId. (The RelaxedPrecision decoration doesn't go on the
type.)
This is important because OpLoad and OpStore require the same SpvId on a
variable; you can't OpLoad from one type SpvId and OpStore to a
different type SpvId, even if the underlying type behind the SpvId is
the same.
(A slightly simpler fix exists at http://review.skia.org/437237, but
this triggered a memory pooling bug that I can't properly debug from
this machine.)
Change-Id: I7669a95a2c946dde1eeff73474a3a0fb9d180512
Bug: skia:12248
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/437683
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit 130338c9e1.
Reason for revert: SkSL_ArrayComparison test causes Adreno 630/640 to crash in Vulkan
Original change's description:
> Fix array-of-vector comparisons in Metal.
>
> Comparing `vec1 == vec2` returns a bvec in Metal, so the result must be
> wrapped in `all()` in order to boil it down to a single boolean result.
> Our array-comparison helper function did not do this. Fortunately,
> `all(scalar)` is a no-op, so we can just wrap the result unilaterally.
>
> Change-Id: I4f1f09a6832164ae2e6577d53b317f561332d581
> Bug: skia:12324
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/437736
> Auto-Submit: John Stiles <johnstiles@google.com>
> Commit-Queue: Brian Osman <brianosman@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com,skcq-be@skia-corp.google.com.iam.gserviceaccount.com
Change-Id: Ic76a5527a8339c8201f52df08d43041d7dcbeb61
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:12324
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/438077
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This reverts commit 23d8f94535.
Reason for revert: SkSL_ArrayComparison test causes Adreno 630/640 to crash in Vulkan
Original change's description:
> Fix array-of-matrix/struct comparisons in Metal.
>
> Metal needs helper functions in order to compare arrays, structs, and
> matrices. Depending on the input code, it was possible for the
> array-comparison helper to be emitted before a matrix-comparison
> or struct-comparison helper. If this occurred, array comparisons of that
> matrix or struct type would fail, because the operator== for the array's
> inner type was defined after array==, and Metal (like C++) parses
> top-to-bottom and only considers functions declared above the current
> function.
>
> We now emit prototypes for all the array, struct and matrix helper
> function. These prototypes are emitted above any helper functions. This
> ensures visibility no matter how your comparisons are organized.
>
> Change-Id: Ib3d8828c301fd0fa6c209788f9ea60800371edbe
> Bug: skia:12326
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/437739
> Commit-Queue: John Stiles <johnstiles@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com,skcq-be@skia-corp.google.com.iam.gserviceaccount.com
Change-Id: I9e0fc69c46e1b4f63133e21e130e527ca4f0b31a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:12326
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/438076
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Metal needs helper functions in order to compare arrays, structs, and
matrices. Depending on the input code, it was possible for the
array-comparison helper to be emitted before a matrix-comparison
or struct-comparison helper. If this occurred, array comparisons of that
matrix or struct type would fail, because the operator== for the array's
inner type was defined after array==, and Metal (like C++) parses
top-to-bottom and only considers functions declared above the current
function.
We now emit prototypes for all the array, struct and matrix helper
function. These prototypes are emitted above any helper functions. This
ensures visibility no matter how your comparisons are organized.
Change-Id: Ib3d8828c301fd0fa6c209788f9ea60800371edbe
Bug: skia:12326
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/437739
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Comparing `vec1 == vec2` returns a bvec in Metal, so the result must be
wrapped in `all()` in order to boil it down to a single boolean result.
Our array-comparison helper function did not do this. Fortunately,
`all(scalar)` is a no-op, so we can just wrap the result unilaterally.
Change-Id: I4f1f09a6832164ae2e6577d53b317f561332d581
Bug: skia:12324
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/437736
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Most of the code generated by the fuzzer is nonsense, but there is a
method to its madness. The crash is only triggered under specific
conditions:
- The runtime effect has enough helper functions to mostly fill up the
call graph hash-map. It won't rehash until it gets close to capacity.
- There must be several calls to built-in functions, in order to add
elements to the call graph to force a rehash.
The fuzzer-generated code manages to satisfy both these requirements.
Change-Id: I9a1d7535557fedd4e9bfece3930ac86ede291ffe
Bug: oss-fuzz:36655
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/437118
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
GLSL allows an array of `lowp float` to be compared against `highp
float` seamlessly because the types are considered to be the same. SkSL,
however, treats these as different types, so we need to coerce the types
to allow this comparison to work.
In other words, these comparisons can cause an array to be implicitly
casted. The expression `myHalf2Array == float[2](a, b)` should be
allowed when narrowing conversions are enabled. To allow this to work,
we need a dedicated IR node representing this type coercion.
We now allow implicit coercion of array types when the array's component
types would be implicitly coercible, and have a new IR node representing
that implicit conversion.
This CL fixes array comparisons, but array assignment needs additional
fixes. It currently results in:
"type mismatch: '=' cannot operate on (types)".
Bug: skia:12248
Change-Id: I99062486c081f748f65be4b36a3a52e95b559812
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/436571
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
The optimization logic for swizzling a constructor assumed that every
argument to the constructor was a scalar or vector. When it was written,
this assumption was true. However, we recently added support for casting
mat2x2 to float4 which violates the assumption.
We now check every argument and do not attempt to optimize if a
non-scalar, non-vector arg is found.
Change-Id: Ia2b297bd62dfdf4af56712164fbc80c29c9611eb
Bug: oss-fuzz:36852
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/437017
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
These parse into new modifier bits; the IR generator does not yet
support these bits. That's coming in a followup CL.
Change-Id: I362e9227694f9b862eaad100f6afca45a9b62a01
Bug: skia:12248
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/436336
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
We don't currently support this. There's no explicit syntax to cast an
array's type, but it can be implicitly required in some situations, like
`halfArray == floatArray` (when fAllowNarrowingConversions is on).
Change-Id: I00fe0ddd4f2682b2950e828dd78bb941d5f0430e
Bug: skia:12248
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/436560
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
SPIR-V code generation synthesizes some extra variables that don't
actually exist in the Program. Checking the ProgramUsage of these
variables would fail; ProgramUsage::get doesn't know about these
variables, so it asserts (and would consider them as dead even if it
didn't assert). We now track our SPIR-V bonus variables in a separate
set, and always report them as live.
Change-Id: If2f681470654025abf7ca4b3ec8126de2eb01297
Bug: oss-fuzz:36770
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/435625
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I7b53df1eae83a596c4d1f3620e7f9bd146f68af2
Bug: oss-fuzz:36655
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/434465
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
At present, they aren't hooked up to anything. They will be made
functional in followup CLs.
Change-Id: I4bfc25eb4e19fce4c36ea0b55494bf37b2a9ee23
Bug: skia:12248
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/430637
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Boolean vector expressions already allowed swizzles (see the
SwizzleBoolConstants.sksl test), but scalars had been inadvertently
disallowed.
Change-Id: I89e7139db50981f0ee1a9a5086b02603e57f967d
Bug: skia:12195
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/427196
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Boolean scalar-swizzling is currently not working.
Change-Id: Icd965e4b64a12311d098168f65622110d5fb3437
Bug: skia:12195
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/427038
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
The tests now check bool4-mat2 conversions, which fortunately do work,
and the vector-to-matrix tests include int and bool conversions as well.
Change-Id: I971271838a93081b9258deb7c1d13b7732fb2440
Bug: skia:12067
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/426757
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
The fuzzer quickly discovered that the newly introduced mat2-to-vec4
conversion code did not account for integer vectors. We now handle
`ivec4(mat2)` casts properly. This required some non-trivial
restructuring of the logic, but in the vast majority of cases, the types
will match and the end result will be identical.
Change-Id: If07c2fe4b4345bd767384b1802374910f65cd3f0
Bug: oss-fuzz:35998
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/426756
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
GLSL supports casting vec4 into mat2 and vice versa, so SkSL should have
equivalent support. This CL allows the Compound constructor to take a
matrix as input, and fixes up backends to do the right thing when a
matrix shows up in the compound-constructor path.
Change-Id: I13289ad0a27ba59bddc3706093820594efebc693
Bug: skia:12067
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/426003
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Change-Id: Ic1a60da6359a5eb91da667340fdfdb5c7952d7c7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/426440
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Bug: skia:11296
Change-Id: I7d41614957d6fa535faadebbeca890b54b6977ac
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/425996
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This should generate the same output as before, except that SpvIds are
renumbered. We now use `writeComposite` instead of manually emitting
SpvOpCompositeConstruct instructions, and some logic for column-building
was simplified; columns counts are no longer tracked as a separate
value, since we can just call size() to inspect this.
Change-Id: If273341a0938eb5f7a6e2db12b080c7d0dae600a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/426060
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This reverts commit a89781215a.
Reason for revert: breakage on Windows
Original change's description:
> Add tests for matrix-vector conversions.
>
> GLSL supports casting vec4 into mat2 and vice versa, so SkSL should have
> equivalent support. Adding tests as a starting point.
>
> Change-Id: If8bcbf99afcec94d948d5da9e6205cb4a232af18
> Bug: skia:12067
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/425837
> Auto-Submit: John Stiles <johnstiles@google.com>
> Commit-Queue: Brian Osman <brianosman@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I2563041f538b1b20074385f1b61af5fc506ffad5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:12067
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/426057
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
GLSL supports casting vec4 into mat2 and vice versa, so SkSL should have
equivalent support. Adding tests as a starting point.
Change-Id: If8bcbf99afcec94d948d5da9e6205cb4a232af18
Bug: skia:12067
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/425837
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reland works around Adreno issue with this formulation of sk_Clockwise:
(sk_RTFlip.y < 0.0 ? !gl_FrontFacing : gl_FrontFacing)
and instead adds this to the top of the function:
bool sk_Clockwise = gl_FrontFacing;
if (sk_RTFlip.y < 0.0) {
sk_Clockwise = !sk_Clockwise;
}
Original description:
SkSL language features that are origin sensitive now use a uniform
to conditionally flip their result rather than generating different
code.
Previously we would insert a "rt height" uniform if sk_FragCoord needed
to be flipped. sk_FragCoord,y was implemented as "realFragCoord.y" or
"rtHeight - realFragCoord.y" depending on SkSL::ProgramSettings::fFlipY.
Now we instead use a two component vector rtFlip and sk_FragCoord.y is
always "rtFlip.x + rtFlip.y*realFragCoord.y". We configure rtFlip as
either (0, 1) or (rtHeight, -1). sk_Clockwise and dFdy simiarly use
rtFlip.y to emit code that always works with either origin.
Bug: skia:12037
Change-Id: I3a2ad6f5667eb4dcd823b939abd5698f89b58929
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/425178
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
This reverts commit 943108b0b2.
Reason for revert: clockwise GM bad on android.
Original change's description:
> Don't key progams/pipelines on origin.
>
> SkSL language features that are origin sensitive now use a uniform
> to conditionally flip their result rather than generating different
> code.
>
> Previously we would insert a "rt height" uniform if sk_FragCoord needed
> to be flipped. sk_FragCoord,y was implemented as "realFragCoord.y" or
> "rtHeight - realFragCoord.y" depending on SkSL::ProgramSettings::fFlipY.
>
> Now we instead use a two component vector rtFlip and sk_FragCoord.y is
> always "rtFlip.x + rtFlip.y*realFragCoord.y". We configure rtFlip as
> either (0, 1) or (rtHeight, -1). sk_Clockwise and dFdy simiarly use
> rtFlip.y to emit code that always works with either origin.
>
> Bug: skia:12037
>
> Change-Id: I7a09d0caac60a58d72b76645ff31bcabde4086b6
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/414796
> Commit-Queue: Brian Salomon <bsalomon@google.com>
> Reviewed-by: Greg Daniel <egdaniel@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
TBR=egdaniel@google.com,bsalomon@google.com,ethannicholas@google.com
Change-Id: I91cc0d86be216f6c32e453a231de088c991be4b2
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:12037
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/425056
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
SkSL language features that are origin sensitive now use a uniform
to conditionally flip their result rather than generating different
code.
Previously we would insert a "rt height" uniform if sk_FragCoord needed
to be flipped. sk_FragCoord,y was implemented as "realFragCoord.y" or
"rtHeight - realFragCoord.y" depending on SkSL::ProgramSettings::fFlipY.
Now we instead use a two component vector rtFlip and sk_FragCoord.y is
always "rtFlip.x + rtFlip.y*realFragCoord.y". We configure rtFlip as
either (0, 1) or (rtHeight, -1). sk_Clockwise and dFdy simiarly use
rtFlip.y to emit code that always works with either origin.
Bug: skia:12037
Change-Id: I7a09d0caac60a58d72b76645ff31bcabde4086b6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/414796
Commit-Queue: Brian Salomon <bsalomon@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
We've lived without this for a long time. It bloats sksl_gpu (inhibits
some inlining!), and if things go according to plan, we'll be removing
enum support from SkSL entirely soon.
Change-Id: If844bbe5fdae41df7930d5c8ea9b832f9dd1b922
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/419099
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Add some comments explaining why different APIs flip the way they do.
Remove flip from Metal code generator and reverse the way flipping
occurs in Metal backend.
Change-Id: Ibec71e8d96c66d4b34b5ce6417056674090f979c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/413236
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
Expressions which would overflow/wrap the result type are now left
as-is.
Change-Id: I6a942f337e6e5761823f5c9dcd214fa58227a626
Bug: skia:10932, skia:12050
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/413138
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Expressions which would optimize to a non-finite value are now left
as-is.
Integer math still has problems with overflow, but that can be addressed
separately.
Change-Id: I363a2c42684989062f606186f48246b7ac5b585c
Bug: skia:12050
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/412956
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: Ic6a3ce722c607d4d3c47d37e5749a01ae5fb193f
Bug: skia:12050
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/412668
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I3427fbaf57787c3051db95ec5882c9292d7985cf
Bug: skia:12034
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/411312
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
In the majority of cases, a uniform is an equally good substitute, and
replacing `sqrt(N)` with `unknownInput` actually makes the test clearer.
Change-Id: I7bcb477571972d7aa2ce8c49b3674471f7310748
Bug: skia:12034
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/411306
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This is allowed by GLSL, so we allow it too.
GLSL ES 1.0, Section 6.1: "The idiom “(void)” as a parameter list is
provided for convenience."
Change-Id: I551c505d3de518a75acd5e306f09f0f0767e43f2
Bug: skia:12025
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/411300
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This is a reland of c6260f9742
Problematic DeadReturn.sksl test cases have been moved to DeadReturnES3.
Original change's description:
> Eliminate unreachable code during optimization.
>
> The Adreno 5xx and 6xx previously failed the SkSLStaticSwitchInline
> test; the driver struggled to interpret code with multiple return
> statements in a row. We now detect unreachable statements and eliminate
> them.
>
> Change-Id: I344d632f2488ca65b0635b37bebffe6e4fb607c5
> Bug: skia:12012
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/410256
> Auto-Submit: John Stiles <johnstiles@google.com>
> Commit-Queue: Brian Osman <brianosman@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
Bug: skia:12012
Change-Id: I748e8761cbc71c811b5ad8fe49186f980261d8b9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/410793
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
MetalCodeGen would incorrectly identify `(someMatrix, someScalar)` as a
math operation between `someMatrix` and `someScalar` and attempt to
convert `someScalar` to a matrix. If `someScalar` was a Boolean type,
this would lead to an assertion.
The binary expression is now checked more thoroughly before converting
the scalar into a matrix.
Change-Id: Id7e104d5533d8c43375927d4815b83e1a3c36be1
Bug: skia:11125
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/410682
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This reverts commit c6260f9742.
Reason for revert: Nexus 7 is grumpy, fails SkSLDeadReturn_GPU
https://ci.chromium.org/raw/build/logs.chromium.org/skia/539fff50ff093c11/+/annotations
Original change's description:
> Eliminate unreachable code during optimization.
>
> The Adreno 5xx and 6xx previously failed the SkSLStaticSwitchInline
> test; the driver struggled to interpret code with multiple return
> statements in a row. We now detect unreachable statements and eliminate
> them.
>
> Change-Id: I344d632f2488ca65b0635b37bebffe6e4fb607c5
> Bug: skia:12012
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/410256
> Auto-Submit: John Stiles <johnstiles@google.com>
> Commit-Queue: Brian Osman <brianosman@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: Ia3db1f1b28417e479e2d71a4a6ed94a007e47cf9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:12012
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/410780
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
The Adreno 5xx and 6xx previously failed the SkSLStaticSwitchInline
test; the driver struggled to interpret code with multiple return
statements in a row. We now detect unreachable statements and eliminate
them.
Change-Id: I344d632f2488ca65b0635b37bebffe6e4fb607c5
Bug: skia:12012
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/410256
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
GLSL now emits 4x2 diagonal matrices as:
`(mat4x2(1.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0) * n)`
instead of
`mat4x2(n)`.
This works around a long-standing GLSL bug in both Mesa and glslang that
affects several drivers:
https://github.com/KhronosGroup/glslang/issues/2645
Change-Id: If529d5cd150ce720f436cb3634a2fd3423919278
Bug: skia:12003
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/410137
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Although these tests are simple, it ended up uncovering a legitimate
SPIR-V bug (skia:12009).
Change-Id: Ie89235157256b97626aa6ada4d9d6ba62abc57fa
Bug: skia:12009
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/409299
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
If we encounter code like `return 1; return 2;` we need to synthesize a
label, even though the second return statement isn't actually reachable.
This is harmless and satisfies the SPIR-V validator.
Ideally we'd eliminate the dead code entirely, but this case is rare and
isn't likely to cause any problems as-is.
Change-Id: I2d6219dff6868011353e19a662301bec44a015d6
Bug: skia:12009
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/409402
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Now that the various Metal and SPIR-V bugs have been shaken out, we can
enable these tests. Knock on wood.
Change-Id: If4b4e302cfdd91464aaf00bc9639989de5e49aac
Bug: skia:11985
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/408640
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
All the pieces of the puzzle were already here to support componentwise
addition and subtraction of matrices. Division was just a forgotten gap
in the implementation and is now patched up to match + and -.
NOTE: if you read the SPIR-V output very closely, you may be surprised
that there are fewer FDiv operations than you'd expect from reading the
input SkSL. As it turns out, a preexisting optimization is rewriting
`mat / 4` into `mat * 0.25` (see line 2689), and this rewritten form can
use the dedicated MatrixTimesScalar op. So we only get componentwise
FDivs for the `4 / mat` lines in the source.
Change-Id: I011c859f5b3a031fbb95a2956f1194a5f3b3794b
Bug: skia:11985
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/408639
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Multiplication is handled just as before. Ops other than multiplication
are handled by splatting the scalar into a matrix, then performing the
op as a componentwise matrix-op-matrix binary expression.
Change-Id: I654715c45bf5c91b8e9660fdf1c1c6d6818b621a
Bug: skia:11985
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/408637
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
The mat4x2 portion of the test no longer checks its results, as there
are bugs in the Intel and Radeon GPU drivers that prevent mat4x2s
from being constructed properly.
The SkSL optimizer ends up eliminating the 4x2 matrix entirely because
it is unused by the rest of the code, as well at the 4x4 matrix which is
calculated from the 4x2. At this point, I'm OK with this.
Change-Id: If1464f9e4938b0a37b2ec180c686972389d94e83
Bug: skia:12003
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/408900
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Previously, it was structured like Matrices.sksl and had no verification
of its results. Now it double-checks that its outputs match our
expectations. The test is still quite simple for now, however.
Change-Id: Iaa45fe58beb497a63801833f8ba5a493a61139d9
Bug: skia:11985, skia:12003
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/408646
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This reverts commit 5d61cc2f87.
Reason for revert: break Vk bots
Original change's description:
> Vectorize scalars in SPIR-V using ConstructorSplat.
>
> This avoids redundant code, and has a small side benefit of
> deduplicating constant vectors which appear more than once in the code,
> since `writeConstructorSplat` already supports this.
>
> Change-Id: I2972ee922ac92adeb40bc765da3b490a59b957b3
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/408360
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I9fc0b896c0cfccc348d510a02df47d5ad74a0e90
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/408644
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
This test was generating invalid code in Metal
(http://review.skia.org/408356), but we didn't catch it because the code
wasn't actually being run.
Change-Id: I649034593a566f9e835b1cf7b0702c64952d31ef
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/408641
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Support for constant-expression function calls in SkSL now exists, and
support for abs() was added at http://review.skia.org/405676.
Change-Id: I3144af993db93a3d640971734d4cb03e0cfb8589
Bug: skia:10835
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/408642
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This avoids redundant code, and has a small side benefit of
deduplicating constant vectors which appear more than once in the code,
since `writeConstructorSplat` already supports this.
Change-Id: I2972ee922ac92adeb40bc765da3b490a59b957b3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/408360
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
We were emitting this at global scope (not in Globals). That would lead
to errors about the variable needing to be in the constant address
space. (You can see the result in ConstArray.metal - the old code was
invalid). Also, we were already making references use _globals, so the
code was double-wrong (or half-right, depending on your perspective).
After the core change, writeVarDeclaration was only used for local
scope, and writeModifiers never used the 'globalContext' parameter.
The removal of finishLine() changed every test output, unfortunately.
Change-Id: Icc1356ba2cc3c339b2f5759b3d18523fd39395bc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/408356
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This reverts commit 90508f02dc.
Reason for revert: avoiding driver bugs this time
Original change's description:
> Revert "The Matrices test now verifies its results."
>
> This reverts commit 86121f6c0e.
>
> Reason for revert: tree sad
>
> Original change's description:
> > The Matrices test now verifies its results.
> >
> > Previously, this test did a bunch of matrix math but never actually
> > checked its results for correctness.
> >
> > Change-Id: I353be58049286266c2d561b0939b3874d2684403
> > Bug: skia:11985
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/407360
> > Commit-Queue: John Stiles <johnstiles@google.com>
> > Commit-Queue: Brian Osman <brianosman@google.com>
> > Auto-Submit: John Stiles <johnstiles@google.com>
> > Reviewed-by: Brian Osman <brianosman@google.com>
>
> TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
>
> Change-Id: I1335f01c14ee955426e02efaa3c30421cd41aa34
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: skia:11985
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/407617
> Reviewed-by: John Stiles <johnstiles@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
Bug: skia:11985
Change-Id: I214375d74977f324973da72c440d7ff5ff179016
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/408157
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This reverts commit 86121f6c0e.
Reason for revert: tree sad
Original change's description:
> The Matrices test now verifies its results.
>
> Previously, this test did a bunch of matrix math but never actually
> checked its results for correctness.
>
> Change-Id: I353be58049286266c2d561b0939b3874d2684403
> Bug: skia:11985
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/407360
> Commit-Queue: John Stiles <johnstiles@google.com>
> Commit-Queue: Brian Osman <brianosman@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I1335f01c14ee955426e02efaa3c30421cd41aa34
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:11985
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/407617
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Previously, this test did a bunch of matrix math but never actually
checked its results for correctness.
Change-Id: I353be58049286266c2d561b0939b3874d2684403
Bug: skia:11985
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/407360
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
In particular, this optimizes abs() and sign() when all inputs are known
at compile time. This resolves a TODO on a test case in
`IllegalIndexing.rts`.
Change-Id: Ica310522a85b42dc7ae255bd25004a6629d04176
Bug: skia:10835
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/405676
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
A constructor like `float3(five)` is not a compile-time constant, so we
miss optimization opportunities like folding. Constant variables inside
splat constructors are now replaced when optimization is on, so this
would optimize down to `float3(5.0)` and be eligible for folding.
Change-Id: I4bf6f52a48ef733e6b24791d02687081194ef488
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/404676
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
