Change-Id: Ic2d1240ab785101365b0fd934562505fb5a3e599
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/381816
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Expressions like `value == value` or `color.a != color.a` can be
replaced by `true` or `false` on sight. The GLSL spec makes it clear
that checking for NaN is optional:
4.7.1 Range and Precision
"... NaNs are not required to be generated. Support for signaling NaNs
is not required and exceptions are never raised. Operations and built-in
functions that operate on a NaN are not required to return a NaN as the
result."
Change-Id: I2ad9f2dc505b638ea2904bef41b7a79a2b329551
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/381262
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This CL will be used to test for potential performance regressions (or
improvements) that we might cause by disabling this optimization pass.
It will be reverted in ~1 day.
Change-Id: I26b7687c341eb6d81231406381c39869cfccf6d6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/381259
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I7a7874e58bf53978afce8a41b26092406b6490ed
Bug: skia:11342
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/380360
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Expressions like `x * 1`, `x *= 1`, `x + 0`, `x * 0`, or `0 / x` don't
actually do anything, and can be simplified to just `x` or `0`. (The
zero case must also check that `x` doesn't have side effects, because
`0 * myFunction()` still needs to call `myFunction`.)
`0 - x` is also detected and rewritten as `-x`.
`0 / 0` is left as-is.
This logic works for scalars and vectors; matrices are left as-is.
A similar optimization also occurs in the constant-propagation pass, so
we see almost no diffs in the tests. If control-flow analysis is turned
off, we do see some improvements. (I didn't reuse the existing code at
all, since it was designed around rewriting the CFG tree, but the
concept was identical.)
Change-Id: Ia99cd81f1d4cd3dafaa43ccac6a2261e3257a185
Bug: skia:11343
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/380356
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This enables the ternary to be optimized away in code like:
const bool SHINY = true;
color = SHINY ? add_shine(x) : x; // to --> `color = add_shine(x);`
Without constant propagation.
Also, I added a unit test for ternary expression simplification; I
wasn't able to find an existing one.
When the optimization flag is disabled, this CL actually removes the
optimization of `true ? x : y` --> `x` entirely; previously, this
substitution would be made regardless of optimization settings.
Change-Id: I93a8b9d4027902d35f8a19cfd6417170b209d056
Bug: skia:11343
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379297
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Swizzle optimizations now occur at IR generation time. These
optimizations are redundant with the control-flow optimization phase so
they are mostly not visible in our test output, but they do affect DSL
test results. Interestingly, they do improve our test output slightly
as well, for various reasons (e.g. we do not fully optimize lvalues in
the control-flow pass).
Change-Id: I6ebe6d71a5c22d9823b5fa500e43078915cbfb45
Bug: skia:11343
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372257
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This check now runs at function finalization time, before constant
propagation has occurred; this affected the "DeadIfStatement" test.
Our detection isn't smart enough to realize that a loop will run zero
times, so it treats `for` and `while` loops as always running at least
once. This isn't strictly correct, but it actually mirrors how the CFG
implementation works anyway. The only downside is that we would not flag
code like `for (i=0; i<0; ++i) { return x; }` as an error.
Change-Id: I5e43a6ee3a3993045559f0fb0646d36112543a94
Bug: skia:11377
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379056
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This should be legal, and we support this, but some versions of Android
do not: http://screen/3bkQewHF3xUMn5v There's no point in allowing
these shaders to exist; they can't compile on real-world clients, and
these vardecls are borderline meaningless (as the variables being
declared aren't reachable by any other statements).
Change-Id: Ie1351933c90caee9124eeab8983364ec030b2653
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379584
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This reverts commit 50b1b2b90d.
Reason for revert: ending experiment
Original change's description:
> Disable control-flow analysis in SkSL. (Performance experiment)
>
> This CL will be used to test for potential performance regressions (or
> improvements?) that we might incur by disabling this optimization pass.
>
> It will be reverted in ~1 day.
>
> Change-Id: I775cdb0c95df81fa25ebbd66e4ff01f64c660f68
> Bug: skia:11319
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/378456
> Commit-Queue: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: Ie385a82db237ff5651348d82b9651f8ba09375b9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:11319
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379581
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Covers some common geometry processors, texture effect, etc.
This also rearranges how fp keys are arranged in the overall key. We no
longer include the key size as part of the key - this made no sense.
Instead, we explicitly include the number of children. We also put all
data for one fp before any children, so the tree can be reconstructed
more-or-less top-down.
Finally, added an "addBool" helper that reads nicer than addBits(1)
everywhere.
Bug: skia:11372
Change-Id: I4e35257fb5923d88fe6d7522109a0b3f4c4017d4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379059
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This CL will be used to test for potential performance regressions (or
improvements?) that we might incur by disabling this optimization pass.
It will be reverted in ~1 day.
Change-Id: I775cdb0c95df81fa25ebbd66e4ff01f64c660f68
Bug: skia:11319
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/378456
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This reverts commit e4da7b672f.
Reason for revert: breaks SkSLBench perf test
Original change's description:
> Migrate if-statement simplifyStatement logic to IfStatement::Make.
>
> This performs essentially the same simplifications as before, just at
> a different phase of compilation.
>
> Change-Id: Ia88df6857d4089962505cd1281798fda74fd0b02
> Bug: skia:11343, skia:11319
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/376177
> Commit-Queue: John Stiles <johnstiles@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I0051188ffe69426904066eb60a932435efdc2af8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:11343
Bug: skia:11319
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379062
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
All layout(key) fields include the field name meta-data, and use as few
bits as possible.
Bug: skia:11372
Change-Id: Ie12b3e0d01148457e5ea078cbf7d0a4bff35302e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/378596
Reviewed-by: Brian Salomon <bsalomon@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This performs essentially the same simplifications as before, just at
a different phase of compilation.
Change-Id: Ia88df6857d4089962505cd1281798fda74fd0b02
Bug: skia:11343, skia:11319
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/376177
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Rather than have the inliner own this responsibility, the function
finalizer now detects if a function is supposed to return a value but
never actually does. This will allow us to detect this error case even
if the inliner is disabled. The inliner should no longer encounter
functions that claim to return a value but don't, so it will now assert
if one is encountered. (The inliner still has the logic to handle this
case gracefully, just in case.)
The check is currently very simple and doesn't analyze the structure of
the function, so it won't report cases where some paths return a value
and others don't, e.g. this will pass the test:
int func() { if (something()) return 123; }
(This is good enough to resolve the inliner issue, though, as it only
occurred in functions with no value-returns at all.)
Change-Id: I21f13daffe66c8f2e72932b320ee268ba9207bfa
Bug: oss-fuzz:31469, oss-fuzz:31525, skia:11377
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/377196
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Bug: skia:11356
Change-Id: I16322e6396dc7e7c8c50ba1d39e07311cf3bd346
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/376116
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Interestingly, this improves our codegen even with the optimizer fully
enabled, as apparently statement chains like:
`x = true; x = x; x = x;`
were getting transformed by constant-propagation into:
`x = true; x = true; x = true;`
making them no longer candidates for self-assignment elimination.
Change-Id: I6d94a809e94b01a00fd92459fcbce898b3cbbb11
Bug: skia:11343
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/377100
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
In http://review.skia.org/375776, an optimization was added to the
Inliner, causing it to skip generation of unnecessary temporary
variables. The fuzzer immediately discovered a flaw in this logic: the
"unnecessary" variable was actually used in the rare case that a
function failed to actually return a value. The inliner didn't detect
this case. Of course, this isn't a valid program either, so now we
report the error and cleanly fail.
Change-Id: I1f201cfd33f45cace3be93765a4e214e43a46e69
Bug: oss-fuzz:31469, oss-fuzz:31525
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/377101
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Surprisingly, this actually improved our error detection slightly.
The expression `- -half4(0)` can now be simplified to `half4(0)` at
IR generation time, which allows the constant-folder to detect a
constant zero (and from there, a division by constant zero).
Change-Id: I8c4f6ab522efab5bf98913f9c6a1487b7af39a99
Bug: skia:11342, skia:11343
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/376842
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
For now, just use this to prevent *any* layout qualifiers from appearing
on functions, or their parameters.
Bug: skia:11301
Change-Id: I05d8118c7121048c6ef49695a54e3714a8f8687e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/376796
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This adds Analysis::IsConstantExpression, to determine if an expression
is a constant-expression. It now expands to cover 'const' local and
global variables, because we also enforce that the initializer on those
variables is - in turn - a constant expression.
This fixes 10837 - previously you could initialize a const variable with
a non-constant expression, and we'd emit GLSL that contained that same
pattern, which would fail to compile at the driver level. That should
not be possible any longer.
Bug: skia:10679
Bug: skia:10837
Change-Id: I517820ef4da57fff45768c0b04c55aebc18d3272
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/375856
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
The expression `~123` was making a PrefixExpression of type $intLiteral.
It should be converted to type `int` when the ~ prefix is applied.
This change also changes the output from oss-fuzz:27614. Both programs
are essentially nonsense expressions with no real behavior, so this is
fine.
Change-Id: I586be149ce95136fabee72fdd3473814d54948cf
Bug: oss-fuzz:31410
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/376620
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
These variables were later being eliminated by the dead-code-elimination
pass, so you can't see them directly in the final output, but removing
them affects the name mangling off all future symbols, so it causes an
enormous ripple effect in the diff. And of course, it's a waste of time
and memory to synthesize IRNodes just to destroy them later.
If we disable control-flow analysis, we lose the dead-code-elimination
pass entirely; this change is also beneficial for emitting better code
when optimizations are turned off.
Change-Id: I882b3be4f3fd99b77d99b6abe128f26bb9252c89
Bug: skia:11319
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/375776
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Change-Id: Iafeb13812851271a5262730e9c0642d4469c273f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/375020
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Now, even if a qualifier has a default value, we will know that it
appeared in the text. We can use that to check for redundant qualifiers
(as is being done here), and in the IR generator to prevent any use of
certain qualifiers, depending on context. (eg, runtime effects, wrong
shader stage, on a parameter declaration, etc.)
Bug: skia:11301
Change-Id: I2cd6ad35c2b4c4d6f87ade97e80aea84dc16ee4b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/374616
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Bug: skia:10837
Change-Id: I33da2eb1e723ed04ab62d65c21e54306dd362bed
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372677
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Rename factory function from createGLSLInstance() to makeProgramImpl()
Bug: b/180759848
Bug: skia:11358
Change-Id: I095bdf1f26db5a8192fa8ab59000db4a1d561d96
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/373738
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
The test inputs were removed at http://review.skia.org/360778
Change-Id: Ib2918f3f984cd80463bacb822ef510ee9feb1e77
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/373916
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This splits switch() construction into two stages.
- One version of Make takes an array of case-values and case-statement
lists, and is responsible for reporting errors if the case-values are
not unique or are improperly typed. This is what the IR generator or DSL
will start with on its first encounter with the switch statement.
- The other version of Make takes an array of already-processed
SwitchCases and can assume the invariant that they're all correctly-
typed with unique values. This is what we will have when a statement
is inlined or otherwise cloned. (We still assert this invariant, for
correctness' sake, but in release mode we assume it.)
This CL doesn't perform any optimizations at Make time yet; it does work
equivalent to how `switch` works in the IR generator today. It does
improve duplicate case-label checking slightly; duplicate case labels
are now reported, and duplicate `default:` labels are detected.
(Multiple `default` labels won't pass the parser, but they can be
constructed in DSL.)
Change-Id: I537ce2c8236152d58641fb1793619d66a62c01a8
Bug: skia:11342
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372616
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
These were unused - we always enable the advanced blend equation
extension using "blend_support_all_equations" (if enabling the
extension is required at all).
Change-Id: I95fd6483ec54dfaf983290de95629fe0e86c22e8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/373877
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Chris Dalton <csmartdalton@google.com>
Constant propagation might be going away, but static-switches are likely
here to stay. Avoid conflating the two in this test.
Change-Id: If4b6c99c85f124d3bbc20da858693f09f5e4fd59
Bug: skia:11319
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/374117
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 optimizer now properly recognizes all types of exits from a switch
statement. Break, continue and return are all potential exits and need
to be considered when determining the exit path from the switch.
Previously, dead code elimination was hiding the effects of this bug
from us, but it meant that an optimized switch had the potential to
generate lots of worthless IR nodes which then needed to be detected and
eliminated by the CFG. In particular, this affected the enum form of
blend, causing a catastrophic amount of extra work to be done.
Change-Id: If857e38cadfc016884624ea4db25a273ad3dce5b
Bug: skia:11352
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372958
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I672345116e3b5538c0f7e8c5f2f74aa56bb81e6d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372676
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
When we detect a static switch, the optimizer finds the matching switch-
case and eliminates all the other switch-cases. It handles case
fall-through by scanning forward and looking for an unconditional break.
However, the inliner has an interesting quirk--it can replace `return`
statements inside of a switch with `continue` statements, since the body
of the inlined function has been wrapped with a for-loop to allow for
early exits. The optimizer does not recognize these continue statements
as exits from the switch (although they certainly qualify), so it
treats continues as fallen-through and keeps emitting switch-cases.
The dead-code elimination pass was actually doing us a favor here and
eliminating the excess code later. A flag was added to disable DCE in
order to reveal the problem in a test.
Change-Id: I8ff19fde5e32d0ab73d7c5411da40cb953a446f5
Bug: skia:11352
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372956
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
There are two forms. Swizzle::Make supports components XYZW only;
Swizzle::MakeWith01 also supports the 01 components, and restructures
the zeros and ones into a constructor (as IRGenerator::convertSwizzle
has historically done). This means that once we are past the initial
IR generation stage, and we know that the 01 components have been
eliminated, we can avoid the extra 01-handling logic and just call
Swizzle::Make directly. This isn't a huge deal but it means that call
sites like the inliner can avoid some extra work that will never happen.
Change-Id: I46690c3d6b07feb6327ee72e8f66f15592a35554
Bug: skia:11342
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/371398
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>
Surprisingly, this error is actually caught by our parser, which
interprets the default label in a unique way. From the parser comments:
"Requiring default: to be last (in defiance of C and GLSL) was a
deliberate decision. Other parts of the compiler may rely upon this
assumption."
The comment is true--we don't check for duplicate default switch-case
labels anywhere else in the code, just here in the parser.
We rely on this, so we should have a test for it.
Change-Id: I6df5c565aca4d4b8565b96638dce9504efc39ccc
Bug: skia:11340
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372617
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I4df18946cdb3d9f1f7833461f913f2df94696821
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372197
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
These were all unused, and only implemented on one backend.
Change-Id: Ibd2fcef1a971e6c1bd9da0784c5d852a60708484
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372117
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Change-Id: I885149c73be63c223ac88a697ffe046a7f8384d0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372116
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Bug: skia:11335
Change-Id: I88c952cbfe2d2c5920e17675da1674928f37b982
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/371480
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Bug: skia:11295
Change-Id: Iec11f3f4d26eb5b1c07707b3cedd09096bad80d0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/371478
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
When coercing a type, we would previously call checkValid() so we could
detect function-references and type-references, so we could get a nicer
error message.
It turns out that we can just do the "is this a type-reference/
function-reference?" check directly inside coerce() and get the same
improved error messages. Since we should be coercing all our values to
the right type, and type/function-references aren't coercible to
anything, this should catch them all. I don't expect any of these
to survive all the way to the end of IR generation.
(In case one of these types does slip through, I've left the error case
in checkValid, but I've also put in an assertion. If the fuzzer can
make that assertion fire, we are probably missing a call to coerce()
somewhere.)
This cleanup is meant to help migrate coerce() out of IRGenerator.
Change-Id: I031809adf439b1766048768b782c57e7f2494006
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/371479
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Adds trivial name mangling to the .stage output, so we can verify that
it's working in all places (declarations, references, etc). Also added
another global variable whose initializer is - in turn - another global.
Bug: skia:11295
Change-Id: Ic220bfae0a6d1eeeba66ade30d3d781af15c5dea
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/371477
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Includes variables with and without initializers. Note that both the
.skvm and .stage output is incorrect right now. (No declarations for
global variables in .stage, and the initializer is dropped in .skvm).
Bug: skia:11295
Change-Id: Icb6d797616be6a1bc7cbdc9db4fefa7e30c65656
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/371143
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
None of these are legal in GLSL ES 1.0. Added a new test that previously
compiled without error. Started out with just assignment and equality,
then realized that sequence and ternary should be blocked, too.
Bug: skia:11323
Change-Id: I02691f819565afabeadbb12cab6c07acf40093f7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370880
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
When SPIR-V generates function calls to an intrinsic, it assumes that
it can get a pointer to out-parameters referenced by the intrinsic.
This does not account for swizzled out-parameters; these are valid
lvalues, but do not work with getPointer().
The two intrinsics supported by SkSL which have an out-parameter are
frexp and modf, so these tests were fleshed out to trigger the error.
Neither of these are supported in ES2, though, so we cannot test them
via Runtime Effects.
Change-Id: Ib92707a28ba6d1c282d20e29a2a387bddf74ad23
Bug: skia:11052
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370116
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
The out-param helpers emitted by the Metal code gen (intended to provide
GLSL out-parameter semantics in Metal) emitted bad code if passed the
same variable for two separate out parameters. It would previously
create two parameters in the helper with the same name. The helper
function now omits the name of the second variable in the parameter list
if it is redundant; we already know the caller is passing the same
variable twice.
Change-Id: Ibdc6c02a9e9e4bdb4f4546a25068f2018aa07b10
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370258
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
GLSL ES2 documentation on out parameters: "Evaluation of an out
parameter results in an l-value that is used to copy out a value when
the function returns."
The inliner does not do any alias checking when inlining an `out` param.
That is, passing the same variable to two separate `out` parameters
would not generate two distinct lvalues in the inlined code; it reuses
the same variable for each out-params in the inlined code.
(Amusingly, our CFG can fully optimize away this test code so it just
returns "red".)
Change-Id: Ib781d2cfdac54f01b6abe159af0c84ff24ff6976
Bug: skia:11326
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370256
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Multi-dimensional arrays aren't legal in GLSL/SkSL, so this should be
caught and flagged as an error. The parser now verifies that a
variable's type isn't an array-type before accepting a `[` token to
open an array on the variable name.
This CL also refactors the IR generator's `convertArraySize` method to
make sure that various checks are made for all callers. Originally this
restructuring was used to verify array multi-dimensionality, but that
didn't detect errors inside struct declarations (which get no error
checking inside the IR generator) so the IR generator updates no longer
need to check the array dimensions.
Bug: skia:11322
Change-Id: Id33f4bdfb544019ddf995a8196c3c09cfe5a4525
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/369916
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
We now interpret any statement of the form `Type identifier...` as a
var-declaration and report errors as such. Previously, if a var-decl
statement generated an error during parse, we'd report errors as if it
were an expression-statement, which meant that slightly-invalid code
could return out-of-context, misleading errors.
Bug: skia:11287
Change-Id: I2c6cf2984760eb34593c80cb30f8c4e007d42027
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370036
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>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Bug: skia:11314
Change-Id: I66476543462ae378a5bfb6cbd902dfa2f5fc45f5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/369917
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
The CL at http://review.skia.org/366399 introduced a bug with
LValue::getPointer. Specifically, getPointer used to return zero when
no pointer is available. (This happens when the LValue is a swizzle.)
That CL changed the error code to -1. However, it did not fix up all
the call sites that checked the return value of getPointer().
This CL fixes up those call sites to use -1 consistently, and adds
TODOs in spots which do not check the result from getPointer() at all
(instead assuming it cannot fail). This will allow swizzled out-
parameters to work in SPIR-V as they did before. (Except in intrinsics,
where they seem to have been broken all along, but those are now marked
with a TODO at least.)
Note that we still do not fully emulate GLSL semantics for out
parameters, as out-parameters should only be copied back to the original
variable at the end of the function call to be fully GLSL compliant.
(This CL also replaces a tuple with a named struct for readability.)
Change-Id: I708dc7a69296a4244ba9ceb85c3e68d1f331bbc9
Bug: skia:11052
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368618
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Uses the pipeline-stage callback mechanism. It mangles the type name
(with a test to verify that this works), and then calls defineStruct
with the entire SkSL struct definition string.
Bug: skia:10939
Change-Id: If14cf1b11faaa80ad8d4086cdacf68532bac43fc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368809
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This was being set to zero instead of one by mistake. Interestingly,
this was undetected by the CPU backend, but appears to matter sometimes
on the GPU side.
Change-Id: If827863f69c140f933696c6ff55c8a7095620c29
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368858
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This is just like mul(F32,F32) but optimizes 0*x == 0.
Use it in SkSLVMGenerator; sksl already applies this optimization.
PS2 has a sneaky version using % as a fast_mul() operator, and
PS3 has a sneakier version using ** instead.
We could of course write this all out using fast_mul() the long way,
but I found that quickly became difficult to read.
Change-Id: Iae35ce54411abc00e7729e178eb6a10f151a5304
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368838
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Fixes another instance of anglebug.com/2098 with advanced blend
functions.
Change-Id: I91863723d8b4c33ab2f5a527fe0374e8947bba16
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368813
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Disabled on Adreno 5xx/6xx as the tests do not pass on those GPUs:
http://screen/3Dkgs9syj37cjBV
Change-Id: Ib935d01e8f06dbfe7decd5cc4e52e0688b48be08
Bug: skia:11306, skia:11308
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368805
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit 4908a24d4b.
Reason for revert: test fails on Adreno 5xx/6xx, will land tests
separately and disable on Adreno
Original change's description:
> Revert "Add support for matrix == and != in Metal shaders."
>
> This reverts commit c501857188.
>
> Reason for revert: breaking many bots
>
> Original change's description:
> > Add support for matrix == and != in Metal shaders.
> >
> > We need to polyfill an operator== and != when these are first
> > encountered in the code.
> >
> > Change-Id: I539c838ee1871bcb0c4b66abb8a4a0f91146cd4f
> > Bug: skia:11306
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368496
> > 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: Id583109a0d167c2c58a57644b14cd5f49d670737
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: skia:11306
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368801
> Reviewed-by: Greg Daniel <egdaniel@google.com>
> Commit-Queue: Greg Daniel <egdaniel@google.com>
TBR=egdaniel@google.com,brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Bug: skia:11306
Change-Id: If7c628b8c7a2ce40d6c88599a7660ff91c4ac67a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368804
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
"Constant" is an address space qualifier and can't be applied to a
local variable. "Const" in GLSL (and hypothetically SkSL) is meant to
apply to a constant expression regardless of address space.
Our previous test was not finding any error because the optimizer was
eliminating the constant expressions entirely.
Change-Id: I6cfe8e2a621c79945b33e0166780d81e79890a1b
Bug: skia:11304
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368517
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit c501857188.
Reason for revert: breaking many bots
Original change's description:
> Add support for matrix == and != in Metal shaders.
>
> We need to polyfill an operator== and != when these are first
> encountered in the code.
>
> Change-Id: I539c838ee1871bcb0c4b66abb8a4a0f91146cd4f
> Bug: skia:11306
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368496
> 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: Id583109a0d167c2c58a57644b14cd5f49d670737
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:11306
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368801
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
We need to polyfill an operator== and != when these are first
encountered in the code.
Change-Id: I539c838ee1871bcb0c4b66abb8a4a0f91146cd4f
Bug: skia:11306
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368496
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This emits SkSL that is more-or-less what the compiler re-ingests when a
runtime effect is used to create a GrFragmentProcessor.
Change-Id: I0926be44fc4493e722a5edc18198e161e4192cde
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/367883
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Currently, SkSL is able to constant-propagate `x = x + constant` into
`x = constant` when the starting value of x is known. However, it is not
able to do the same optimization for `x += constant`. This test
demonstrates that once += is encountered, we lose track of x's value and
can no longer propagate its value.
(This is equally true of all the op-assignment operators, += -=
*= /= etc.)
Change-Id: I3523e96baf9a73982cf3b09f0d23b95adacf106b
Bug: skia:11192
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368248
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
The leftover tests in shared/ are not easily testable as Runtime
Effects; they do things that ES2 doesn't support or use a feature not
exposed directly by Runtime Effects.
Change-Id: I7ebe170cf713c4a0d2dbef333c1fcbac2410c67f
Bug: skia:11009
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/367059
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
These cover new ground; when combined with some additional optimization
work, they can cause crashes in the optimizer that we don't see from any
existing test.
Change-Id: I3958a5522cfe0929d0753e6e617d72e032c7f5a3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/367063
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
SkSL.
It would previously catch 1 / 0, but fail to detect x / 0.
Bug: skia:11051
Change-Id: I3adb5942cce03a7ad40a13a8ca5d5a7f2029d6ad
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/366720
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: Ethan Nicholas <ethannicholas@google.com>
This takes away one of our gadgets for thwarting dead-code elimination
in unit tests, but it's the right thing to do. Comma expression left-
sides without side effects are clearly dead code.
Change-Id: Iaee490b4a742d06a0a0be94cddaa69a51543d8f5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/366719
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This fixes the OutParamsTricky test.
Change-Id: If59637bc946b71b141ae1d90cf1652bf80b163c4
Bug: skia:11269
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/366399
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
- MultipleAssignments
- NegatedVectorLiteral
- NumberCasts
- OutParams
- OutParamsTricky (disabled on GPU due to skia:11269)
Change-Id: I87dc9c5019931f3d2dc3aafbe1e02d0eee2e1a05
Bug: skia:11009, skia:11269
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/366400
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 is enforced by ANGLE in Strict ES2 mode; we need to enforce it as
well.
Change-Id: I6e2f547ad8e0ce817742cf84659764cf6bce38b9
Bug: skia:11270
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/366339
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 reverts commit 38df4c8470.
Reason for revert: updated ArrayTypes test for ES2 compatibility
Original change's description:
> Revert "Improve support for arrays in Metal."
>
> This reverts commit dd904af566.
>
> Reason for revert: breaks ANGLE
>
> Original change's description:
> > Improve support for arrays in Metal.
> >
> > Arrays in Metal now use the `array<T, N>` type instead of the C-style
> > `T[N]` type. This gives them semantics much more in line with GLSL,
> > so they can be initialized and assigned like GLSL arrays.
> >
> > This allows the ArrayTypes and Assignment tests to pass, so they have
> > been added to our dm SkSL tests. (ArrayConstructors also passes, but
> > is not ES2-compliant so it is not enabled.)
> >
> > Change-Id: Id1028311963084befd0e044e11e223af6a064dda
> > Bug: skia:10761, skia:10760, skia:11022, skia:10939
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/365699
> > 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
>
> Change-Id: If6a18dea7d6a45fa7836e9129bf81c2e536f07e3
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: skia:10761
> Bug: skia:10760
> Bug: skia:11022
> Bug: skia:10939
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/365976
> Reviewed-by: John Stiles <johnstiles@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Bug: skia:10761
Bug: skia:10760
Bug: skia:11022
Bug: skia:10939
Change-Id: Ia1c4917f5d3c41162d282b3093814d861707ad30
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/366144
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This reverts commit dd904af566.
Reason for revert: breaks ANGLE
Original change's description:
> Improve support for arrays in Metal.
>
> Arrays in Metal now use the `array<T, N>` type instead of the C-style
> `T[N]` type. This gives them semantics much more in line with GLSL,
> so they can be initialized and assigned like GLSL arrays.
>
> This allows the ArrayTypes and Assignment tests to pass, so they have
> been added to our dm SkSL tests. (ArrayConstructors also passes, but
> is not ES2-compliant so it is not enabled.)
>
> Change-Id: Id1028311963084befd0e044e11e223af6a064dda
> Bug: skia:10761, skia:10760, skia:11022, skia:10939
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/365699
> 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
Change-Id: If6a18dea7d6a45fa7836e9129bf81c2e536f07e3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:10761
Bug: skia:10760
Bug: skia:11022
Bug: skia:10939
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/365976
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Arrays in Metal now use the `array<T, N>` type instead of the C-style
`T[N]` type. This gives them semantics much more in line with GLSL,
so they can be initialized and assigned like GLSL arrays.
This allows the ArrayTypes and Assignment tests to pass, so they have
been added to our dm SkSL tests. (ArrayConstructors also passes, but
is not ES2-compliant so it is not enabled.)
Change-Id: Id1028311963084befd0e044e11e223af6a064dda
Bug: skia:10761, skia:10760, skia:11022, skia:10939
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/365699
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
We now catch this error at IR generation time; previously we'd send it
to the driver (where it would fail to compile).
Change-Id: I45890214ffa164be1c0f359320f942bc4dc479ca
Bug: skia:11265
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/365697
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This uncovered a bug in Metal code generation of `matX *= matY` which is
now fixed. (It was emitting the helper function more than once.)
Change-Id: I0aeb0efe7ab5fbf5592a8ca6f4f5b50354d3d7f4
Bug: skia:11262
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/365489
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 works around a GLSL compilation bug on the Tecno Spark 3 Pro.
Change-Id: I516bd64745a8e99cccc87ee4bb2e1f5d5b26c130
Bug: skia:11255
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/364116
Commit-Queue: Robert Phillips <robertphillips@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
Previously, our only test was invoking `sin(1)` which is a pretty
ineffective test. Now, we test args and return types for all the basic
scalars/vectors/matrices.
Change-Id: I7d335303eef8b9c9c6cfef2265a15bbd9bd73e0c
Bug: skia:11246
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/363943
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
These are basic vector types, required by GLSL ES2, but we could not
create helper functions using them because they were missing from our
GrSLType enum. (This also prevented Runtime Effects from using these
types in helper functions.)
Change-Id: I78c328499e8ed90cb29c641b90ee59460a5a45de
Bug: skia:11246
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/364036
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
These tests have updated to return green on success, or red on failure.
Some tests were modified slightly to conform to ES2 limitations, or
split into separate ES2 and ES3 parts.
Change-Id: Ib47aeca217aef33f3c4b5999d93afed5d42a1e62
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/363876
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
We already had this trick for scalar integers, this extends it to
integer vectors. As with prior work in this area, it would be better to
detect this case and produce an error, but now we at least produce
consistent and well-defined results (rather than undefined signed
integer overflow).
Bug: skia:10932
Bug: oss-fuzz:29494
Change-Id: I45526fe96b6ea42c0e88b9862f6961b316810321
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/363962
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
The test has been split into an ES2 version and ES3 version; the ES2
side omits unsupported integer ops like << >> & | ^ %.
Change-Id: Iba16d469a477809b17a823b1c68ae8937624c68e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/362616
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
SkVMCodeGenerator was lacking support for the comma operator entirely;
this has now been implemented.
Change-Id: I9350f54e6ee52764c620116e6dbfe4ca3e9cd47e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/363096
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This reverts commit f06aeedcf1.
Reason for revert: fract failing on Tegra3
Original change's description:
> Add intrinsic tests for mod() and fract().
>
> These are fully supported by ES2 and will be covered by dm tests.
>
> Change-Id: Iebf4effe8ab928662b55c0bb5b09e8b2a61487ca
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/362460
> 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>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: Ib2071c90addd01e3b299553e020950a89eb01e4d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/363036
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
These are fully supported by ES2 and will be covered by dm tests.
Change-Id: Iebf4effe8ab928662b55c0bb5b09e8b2a61487ca
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/362460
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>
This reverts commit 0492a744a5.
Reason for revert: Intel HD6000 + ANGLE DX9 fails the floor() test.
Original change's description:
> Add some SkSL intrinsics to our dm tests.
>
> This CL adds dm coverage for:
> - abs(half)
> - sign(half)
> - floor
> - ceil
>
> And creates test output for abs(int) and sign(int); these aren't covered
> by dm because they don't exist in ES2 and so are unsupported by Runtime
> Effects.
>
> Change-Id: Ia3e660408cef50dec8fa4b6bdc12906e96179f6e
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/360419
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I62121efee9315b16e61e7d38659b6f629bdf8bd8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/362056
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This CL adds dm coverage for:
- abs(half)
- sign(half)
- floor
- ceil
And creates test output for abs(int) and sign(int); these aren't covered
by dm because they don't exist in ES2 and so are unsupported by Runtime
Effects.
Change-Id: Ia3e660408cef50dec8fa4b6bdc12906e96179f6e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/360419
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, a uniform not wrapped in an interface block would report a
SPIR-V error:
"Variables identified with the Uniform storage class are
used to access transparent buffer backed resources. Such variables must
be typed as OpTypeStruct, or an array of this type..."
Now, the SPIR-V code generator automatically detects such global
variables and synthesizes a struct named _UniformBuffer to hold them.
When these variables are accessed, an OpAccessChain instruction is added
to grab the variable out of the struct.
Change-Id: I5e852d4de01b866c291506cc8cf6eb547f097d66
Bug: skia:11225
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/360776
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, structs that were defined as part of a variable declaration
would end up declared similarly in the generated code. Now, global
variable declarations that include a struct definition generate two
separate program elements.
Bug: skia:11228
Change-Id: Id7ddde6931fe07a250c2c9c46153879005535fb3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/361359
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This allows interface blocks in Metal to compile even if
`layout(binding=...)` is not specified. It will also be used in SPIR-V
in the followup CL, when an interface block is automatically synthesized
for top-level uniforms.
This CL also reorganizes the unit tests around uniforms a bit.
Change-Id: Ia898c536b454dda6f51677e232a8f6e6c3606022
Bug: skia:11225
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/360778
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This is a known deficiency of runtime effects, next step is to fix how
they manage function signatures to solve the problem.
Bug: skia:10939
Change-Id: Id934e0acdf774b03bd6edce78d7b2c077bdeae00
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/360603
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Pre-cleanup as I start looking at how structs are parsed and handled in
the IR.
Bug: skia:11228
Change-Id: I6334d1073211cbbdf69ddffa8df420c45fd59fcc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/361059
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This reverts commit 578f1acbe8.
Reason for revert: updated test to pass on Mac Intel 5100/6000
Original change's description:
> Revert "Add SkSL for-loop control flow test to dm."
>
> This reverts commit a0c266283a.
>
> Reason for revert: failing on Mac Intel
>
> Original change's description:
> > Add SkSL for-loop control flow test to dm.
> >
> > While loops and do-while loops remain untested in dm, as they are not
> > supported in ES2 (and therefore not available in Runtime Effects).
> >
> > Change-Id: I2f1bfccccd571cc4ced096bc18ebbb9ecc9f9b4a
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/359556
> > 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>
>
> TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
>
> Change-Id: I45335d16a695644eaeb8a535298c0efcc616c1ce
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/359840
> Reviewed-by: John Stiles <johnstiles@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I2dc6e870393708a12286658001b723f25a6aec4a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/359856
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This creates a helper function, _entrypoint, which invokes main() and
assigns its result into sk_FragColor. We also make sure to prevent
sk_FragColor from being dead-stripped from the code during IR
generation.
At present this is useful for allowing our SkSL test shaders to compile.
Change-Id: I2d7fab0e1959a77778ffdb18ca569e869bcaeece
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/358525
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This lets us use descriptive names like `colorRed` and `colorGreen`
instead of `half4(1,0,0,1)` and `half4(0,1,0,1)`. It also lets us use
actual unknown values instead of synthesizing sorta-kinda-unknowns by
calling sqrt.
Change-Id: I61481c33b7ff42182955777b05cfa5fcc13e0efc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/359567
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This allows uniforms to be specified without an explicit `layout(set=N)`
modifier. They will assume a default set value instead.
This turns out to fix a handful of tests in Metal/SPIR-V which were
written with GLSL in mind, or adapted from real generated GLSL code, and
didn't have layout information specified on their uniforms. It will also
make it easier to write SkSL tests using uniforms that can compile
either as a runtime effect or as plain Metal/SPIR-V code.
Change-Id: Id79ec06f278b913a45c09c2e6211195dc98b42c0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/359838
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Checking each variation can occasionally shake out extra bugs--for
instance, the mix intrinsic in SPIR-V breaks when adding coverage for
its various overloads here. (See skia:11222; this will be addressed in
a separate CL.)
Change-Id: I2c3ca7523e59d4c6cce25a70e081a558afedfb87
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/359758
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>
Previously, we would emit nothing at all, but that is not actually
valid if the Block is a child statement (e.g. the body of a loop).
Now we emit braces for empty blocks, even if the block was unscoped.
Change-Id: I456a8d7d306a3e59d85e39f80b9f15fe3347ea19
Bug: skia:11218
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/359562
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
In most cases, this works properly and a `;` is emitted, but in one
particular case (int x, y;) we get nothing.
Change-Id: If88d92502f6a533284dd4e0f78daedaf1481ff3d
Bug: skia:11218
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/359558
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>
In GLSL and SkSL, control statements don't require explicit braces
around single-statement children. That is, the `match = true` child
statement here doesn't need to be braced.
if (condition) match = true;
Because there are no braces, we never create a Block or a dedicated
SymbolTable here. This is normally not a problem, but the fuzzer
discovered that it can dump things into the symbol table inside a child
statement:
if (condition) int newSymbol;
This becomes problematic because the symbol name now outlives its block.
This means `newSymbol` can be referred to later, which should be illegal
(and can cause the optimizer to blow up since the structure is bogus).
There doesn't seem to be any reason to allow this code to compile; the
user can add an explicit scope here to make it reasonable, and it's
(almost) meaningless to declare a symbol that's instantly going to fall
out of scope. This code is now rejected with an error message.
Change-Id: I44778e5b59652d345b10eecd4c88efbf7d86a5e0
Bug: oss-fuzz:29849
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/358960
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I94094be7163a04bf48e86406230156a5433469b6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/359140
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>
Change-Id: I924ac75b5f8a397f7af7a06925ef0c9deba5c509
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/359141
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>
Change-Id: I8309940f8e40d0e84847ae272830896d010c39de
Bug: skia:11219
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/359138
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 previous change caused varDeclarations() to sometimes return an
expression-statement. This only made sense in the context of being
called from Parser::statement(). Other places which called
varDeclarations() expect vardecls and nothing else.
Change-Id: I562657cadfa20dcd77b527f2dc43dca0c6bf389f
Bug: oss-fuzz:29845
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/358528
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Comparing sqrt(5) against a variable containing sqrt(5) was not working
properly in some versions of Android running Vulkan.
Change-Id: I4f6bbff78a9ba56ec6e222f2037d66b13e3cd635
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/358530
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This is a reland of 4ecab92584
This reland folds in subsequent code cleanups and disables a test that
failed on Android + Vulkan.
Original change's description:
> Run unit tests to verify SkSL folding behavior.
>
> The unit test loads SkSL source files from `resources/sksl`, compiles
> the code, and uses SkRuntimeEffect to render a pixel using the effect.
> If solid green is rendered, the test passes.
>
> Change-Id: I2ccb427a907975ae84aee19d8e68d774b2cb638c
> Bug: skia:11009
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/355983
> Commit-Queue: John Stiles <johnstiles@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
Bug: skia:11009
Change-Id: I09196c8ca3041e8957324a0cbb7f7d6963c6e4e7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/358523
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This reverts commit 4ecab92584.
Reason for revert: breaking all the vulkan bots
Original change's description:
> Run unit tests to verify SkSL folding behavior.
>
> The unit test loads SkSL source files from `resources/sksl`, compiles
> the code, and uses SkRuntimeEffect to render a pixel using the effect.
> If solid green is rendered, the test passes.
>
> Change-Id: I2ccb427a907975ae84aee19d8e68d774b2cb638c
> Bug: skia:11009
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/355983
> 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
Change-Id: Ife32f6c33d9ba7a9580b66eb312cffb249c43cb2
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:11009
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/357780
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
This allows us to write SkSL shaders which are valid both for use as
Runtime Effect, and for compilation with skslc targeting Metal.
Change-Id: I74e125d81865d4092e657a7d9948d2e72054bda5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/357777
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>
Added asserts that verify we don't try to emit the same struct or array
with two different memory layout rules. Some code paths were failing to
inspect the associated variable, leading to incorrect errors about the
attached offsets of members.
Added a test case that triggered that error, and also triggers the new
asserts.
Then, fixed the underlying cause: writing out the struct definition as a
side effect of accessing a member in getLValue().
Bug: skia:11205
Change-Id: I6e5fb76ea918ec9ff10425f2d519ddbc54404b27
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/357436
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
The unit test loads SkSL source files from `resources/sksl`, compiles
the code, and uses SkRuntimeEffect to render a pixel using the effect.
If solid green is rendered, the test passes.
Change-Id: I2ccb427a907975ae84aee19d8e68d774b2cb638c
Bug: skia:11009
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/355983
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This will allow us to load these inputs for unit testing in `dm`.
Change-Id: Id256ba7c30d3ec94b98048e47af44cf9efe580d5
Bug: skia:11009
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/357282
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
A passing test returns solid green. Failing tests are written to
return solid red, but drawing any other color than green can be
interpreted as a test failure.
Additionally, tests which cannot compile as RuntimeEffects (due to
non-ES2-compatible features) have been split into an ES2-compatible part
and an ES3 part.
Change-Id: I3f53121d9de0ae4c4e7f1de3177d067811980b55
Bug: skia:11009
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/356999
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This allows us to roll the Parser back to an earlier state if we need
to do so. This includes:
- rewinding the lexer
- restoring the previous Pushback node
- backing out AST nodes
- backing out errors
This functionality is used to back out of parsing a vardecl if we
discover mid-stream that it is actually an expression statement that
coincidentally starts with the name of a type.
Change-Id: Ia5feb45019693931c1e6870e3ff7a5398924c863
Bug: skia:11198
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/356997
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Like _globals, it's not actually necessary to indirect through a
separate pointer at all. The output struct is now passed by reference
and the additional pointer variable is removed.
(Additionally, renamed _skGlobals back to _globals.)
Change-Id: Id089a20cb751cdaedc48462a52da78ee43783611
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/355632
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
The previous implementation assumed that SkSL expressions do not have
side effects and so treated either side of a Boolean expression as
short-circuitable. That is, `foo() && false` and `false && foo()` would
both be optimized to `false`, eliminating the `foo()` call.
We now check for side effects first. An expression like `expr && false`
can only be optimized to `false` if `expr` has no side effects. (If
`expr` does have side effects, the expression is left as-is.)
Change-Id: I473cf026a8afe35d6a8d9518498f2b26d8996e60
Bug: skia:11162
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/356357
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>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Additionally, restructure the unit test to return a color (green for
pass, red for fail).
Change-Id: Ib1bb6bd8771c72cc751d8d2c65cc14a693166d4c
Bug: skia:11112
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/356301
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 exposed a preexisting error: GrGrSLTypesAreSupported.fp used non-
square matrices, which are not actually present in GrSLType. The test
has been updated to use square matrices.
Change-Id: Ib51141cc14a0c3fcd1c3c3abf378f190d457b95f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/356077
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
We already had support for &&, ||, ^^ but somehow the common cases of
== and != were not implemented in the constant-folder.
This CL also updates the test to return a green/red color on success or
failure, instead of assigning arbitrary numbers into sk_FragColor that
don't mean anything. The long-term plan is to signal success or failure
of each test by color code; we can display these colors as swatches in a
GM slide for testing purposes.
Change-Id: I0810108b3c6b656a60cd8aa64ceefd765eff0157
Bug: skia:11112
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/355984
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This reverts commit b7e836cee9.
Change-Id: I3c39a928ba4a9a2863b616f2a500975294b03860
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/355980
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>
Reviewed-by: Mike Klein <mtklein@google.com>
This reverts commit ebf569004f.
Reason for revert: std::clamp is c++17
Original change's description:
> Support indexing by loop variables in SkVMGenerator
>
> Bug: skia:11096
> Change-Id: I25a91bacf1c3455ac67422fb0e59b9b152c2054a
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/354667
> Commit-Queue: Brian Osman <brianosman@google.com>
> Reviewed-by: Mike Klein <mtklein@google.com>
TBR=mtklein@google.com,brianosman@google.com,johnstiles@google.com
Change-Id: I0590cf7fe626fb59be3381b5e8eb66a9a2a9e8cb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:11096
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/356056
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Bug: skia:11096
Change-Id: I25a91bacf1c3455ac67422fb0e59b9b152c2054a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/354667
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
`globalStruct` is now named `_skGlobals` and is passed around directly
by reference, with no additional helper variable (`_globals`) at all.
Change-Id: Icc5566d2212afd14a4d43700e89f50bedcc8b45f
Bug: skia:11168
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/355717
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
The Inliner likes to move function bodies around; after inlining, code
can inadvertently move upwards, above ProgramElements that the code
relies on. We work around this by always emitting functions last.
Change-Id: Ie5486cc3a79a478920342fb9f578d575486fb4cf
Bug: skia:11186
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/354669
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This enforces an even stricter version of the rules from GLSL ES 1.0
Appendix A, Section 5. Essentially, indices (to arrays, vectors,
matrices) must be made of literals, loop indices, and expressions made
of those two.
Bug: skia:10837
Bug: skia:11096
Change-Id: I437a5ed64da58e24d5991ddbde68859f5214e98b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/354665
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
As far as I know, there shouldn't be a way to introduce a struct or enum
other than at global scope; the keywords are not accepted inside a
function body. In fact, I wasn't able to find a way to exercise these
code paths in practice. But we now have concrete assurance that any
possible type can be cloned into a symbol table safely; all Types are
either built-in (available everywhere by design) or are clonable.
Change-Id: I4b006b6cab995b3e598b683736ab9689828629c9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/354664
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
The inliner discovered that when a binary expression is inlined, its
type is not cloned into the destination's SymbolTable. This meant that
when the inlined-from function was later dead-stripped, the type pointer
would become dangling. Did a quick pass over inlineExpression and
inlineStatement and ensured that types are always copied.
Also found that `copy_if_needed` was making a copy of eligible types
each time one was encountered, instead of making one copy and reusing
it. This is fixed as well.
Change-Id: Iee3259ab038dfb04034bf0110af1909ccffec3de
Bug: oss-fuzz:29444
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/354219
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Unsized arrays are now allowed in exactly one place: On the declaration
of an interface block. This satisfies the one existing use-case, which
is the gl_in (sk_in) declaration for geometry shaders. There is no other
useful scenario, and most of our backends don't support them anyway.
Several spots were using less strict checks when attaching sizes to
arrays, allowing for zero or negative-sized arrays, so those are all
fixed now.
The existing tests that initialize arrays are still a problem, because
Metal doesn't support that (neither does GLES2). Also, ArrayConstructors
has gone from generating an error in the Vulkan backend, to invalid
SPIR-V.
Bug: skia:11013
Bug: skia:11127
Change-Id: Ib08dfe9aeec96bf605661665d6f166419d27e8bc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353817
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Change-Id: Ia3ac338bef376aa1649569b9ebd3f7feb23ffd52
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353936
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This better matches GLSL's type coercion behavior.
Change-Id: I73fcfd8a9e57fd4cdb1692074d73ebd8fb788ac2
Bug: skia:11164
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353712
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I47d02ca63ce64d9cfb3de0888d84b2b8a822f2b5
Bug: skia:11164
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353710
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Literals are still flexible; we still allow `1` to coerce to float.
However, we no longer accept code like `int x = sqrt(2);` or
`int x = 0; float y = x;` without an explicit cast.
Change-Id: Ieb294a4877447e2336252f876e8bc489d1e4a59a
Bug: skia:11164
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353417
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: Icd8982d604881effee31cc1392e2717cb112d06d
Bug: skia:11172
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353629
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:10680
Change-Id: Ic77f7355866363ef476a93d8da180cf53207fa6d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353707
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Also renamed Discard to IllegalStatements, and added testing of while
and do loops.
Change-Id: Ibacf69131267a0436808e2e022ad126704af16ef
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353706
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>
It's not sound to pass undefined (skvm::NA) values into select(),
but this is working today because the F32a argument is 'fixing' it.
The first time through this snippet updating fReturn value,
int i = 0;
for (skvm::Val& slot : currentFunction().fReturnValue) {
slot = select(returnsHere, f32(val[i]), f32(slot)).id;
i++;
}
the call to f32(slot) creates an F32{builder, NA}. We pass that to
select() and that argument's F32a(F32) constructor, resulting in
F32a{builder, NA, 0.0f}. Then when we need that as an F32, we resolve
it as splat(0.0f) because the F32a's id field is NA.
In short, best to remove F32a. :)
Added some SkASSERTs that would have caught this.
Change-Id: I67324cf20ad39ca555e69b9c407f379d14046043
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353838
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
I'm looking to phase out I32a/F32a, and rewriting this expression to
avoid select() makes it easier, making these types unused except in
SkVM.{h,cpp}.
There's no particular reason beyond making that refactor easier to do
this: SkVM can convert select(cond, splat(1), splat(0)) into cond & 1
itself, and once I'm done with removing I32a/F32a, if we prefer select
we should be able to rewrite this back as
dst[i] = skvm::select(i32(src[i]), 1, 0);
Change-Id: I562a112e54fdc2578802db02f6754c64a12798cc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353837
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
The "disallowed" tests are largely allowed in the current code, but all
fail properly in the followup CL.
Change-Id: I8e03570165480b60db9701ac1a782e1124ded56b
Bug: skia:11164
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353617
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, `writeVectorConstructor` did not consider boolean types at
all when converting scalars to a different type. Now, this code reuses
the existing logic from `castScalarTo(Float|SignedInt|UnsignedInt)`
which supports Booleans. Added `castScalarToBoolean` to cover going in
the opposite direction.
Change-Id: I5479ab181b9b721db7fbff0bdc01718ce8f9f9b9
Bug: skia:11171
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353625
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This revealed a gap in our SPIR-V scalar constructor support;
typecasting a number to bool would lead to an ABORT.
Change-Id: Idac6d7ba34adfd214ed3cad8139e22d7170456f0
Bug: skia:11172
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353628
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I350a6768ac124362b0d3e0f17e7a026265acf804
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353627
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
The test diffs look scary, but the only actual change is a minor
renumbering of IDs. The actual logic is the same.
Change-Id: I5ecc26c8581a4c01834932ff0291deba7d9e4618
Bug: skia:11171
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353622
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Bug: skia:11094
Change-Id: I68a08e79d29579901b74daca3c22f5112fbb3c8c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353356
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
These need to change because type coercion in SkSL is about to become
more strict in a followup CL; we are disallowing expressions that mix
ints and floats without a cast.
Change-Id: I0f6c3cba53fb67078f447345338262c153236c51
Bug: skia:11164
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353102
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Note that GLSL accepts these sorts of constructors natively, but Metal
and SPIR-V do not. In the generated IR we actually add a cast for
subexpressions where the type does not match. These casts can be seen in
the final output for both GLSL (where they are no-ops) and Metal/SPIR-V
(where they are essential).
This change exposed some missing SPIR-V functionality (vector casts do
not support bool types). This can be fixed up in a followup CL; these
casts were previously disallowed by SkSL entirely, so there won't be any
of them in existing code.
Change-Id: I54ae922e91b38bed032537496428747a081dc774
Bug: skia:11164, skia:11171
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353576
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
GLSL allows mixed types inside a vector constructor, but SkSL currently
doesn't handle it well; some cases don't compile, and others generate
bad code. This will be fixed in a followup CL.
Change-Id: Ia98b498f320b8fa91595404730f6cdc836615140
Bug: skia:11164
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353577
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I9859081a14b110731f943e09fdd94dc10e0c9dfc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353580
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
I started unpacking the mechanics of type coercion, and realized that
the second half of the function was looking up the Symbol for a Type
based on its name (Types are already Symbols), converting that Symbol
back into a Type (we started with a Type anyway), wrapping that Type
in a TypeReference, then calling that TypeReference (which always
calls convertConstructor).
This CL cuts out the middle steps and simply calls convertConstructor
directly. A test was added to confirm that an earlier error encountered
on the CQ is no longer occurring.
Change-Id: I76aae455a301afe4e67ef989d9dfe11f47ed36ae
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353105
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Change-Id: I96b547de4fe4b73096fb26d0ef21a4e7555ca06a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/352238
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This CL also adds tests for vector*scalar and scalar*vector folding.
We currently do not constant-fold these, but support will be added in a
followup CL.
Change-Id: I68d7374ae15ab2f4d805a095803b645c92fb03d9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/352237
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This optimization doesn't perceptibly improve the generated code; it
just replaces a binary expression with an equivalent unary one.
Change-Id: Ib6cd2732a22c26978665c57ee00d7b5e5d0a0aee
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/352123
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This makes almost all existing code read more clearly.
Change-Id: I314331d9aa2ecb4664557efc7972e1a820afc250
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/352085
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
The CFG/definition map are no longer valid after replacing an expression
entirely. Swizzle-of-swizzle optimization was another case where the
optimizer would replace an expression wholesale, but failed to set the
needs-rescan flag.
Change-Id: Ida0363d738cd1d3ac2a48c824aa04065a7ca16b7
Bug: oss-fuzz:29085
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/351776
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Change-Id: I75f907ca673ee67f5d623b032128b97833070a0b
Bug: skia:10931
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/351504
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: If6b23d03b02028b51f96e97080cbd7d34cc33b8f
Bug: skia:10931
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/351503
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 will flatten out expressions such as `!false` or `!true`. We
already had a similar fix-up at IR generation time which handled simple
cases, but this will catch more complicated ones like `!sk_Caps.xxxxx`
(since caps bits are only flattened out at constant propagation time).
Change-Id: I04282809d9a784266a64dbcafd097f3b0662806c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/351497
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 is not actually necessary now that constantPropagate can fully
flatten out unary negation into its constant operands. The compilation
results don't change at all.
Change-Id: I7ab55bd3720413609d799dd866e1703973cb2626
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/351202
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 fixes SPIR-V code generation when encountering nested constructors
like `float3 v4 = float3(float2(1), 1.0);` as featured in our unit test
VectorConstructors.sksl.
Change-Id: I3a0c4b466b3cb17ba50bd264f899e59c55c768ed
Bug: skia:11141
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/350032
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Many of our shaders generate the same vector constant dozens of times,
e.g. Gaussian blur uses float4(1) repeatedly. This change avoids
re-emitting redundant vector constants.
Change-Id: I22a71cd8b2783fb997f52d485b49031f64ca6d96
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/350701
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Previously, we had constant-value deduplication, based on the SkSL type
of the constant. However, we were still generating redundant constants,
because we would emit a separate constant for Float(n) and Half(n), or
Int(n) and Short(n), even though we generate the exact same instruction
for these constants. We now deduplicate based on the type's number-kind,
separating constant literals into three categories: floats, signed ints,
and unsigned ints. This better matches our type-handling in
getActualType.
Change-Id: I5777d4b3d567839b7aa72dc8de76908c18fc387e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/350031
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: Ia4a1c38161046b94dc56a1a76704766f1e14aab7
Bug: skia:11131
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/350019
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, the IR generator had code which could simplify conversion
constructors like `int(1.23)`. Separately, the optimizer's constant
propagation pass had its own separate implementation of these
simplifications as well.
This CL unifies the two implementations. Previously, the constant-
propagation pass version of the code only supported integer literals, so
this change also improves our code generation slightly.
Change-Id: I32c70a5f2aed210d03bef3166b1178a2d40cdabd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/350024
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously `number(boolean)` casts were converted to a ternary during
IR generation, and `boolean(number)` casts caused an error.
Metal and GLSL should support this cast as written. SPIR-V needed a
little bit of logic to handle converting the boolean to a number via
OpSelect.
Change-Id: I0069781e2b5a26a25c8625ab41c2392342bfd10d
Bug: skia:11131
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/349066
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
The new unit test demonstrates load/store reordering is error-prone.
At head we're allowing loads from a given pointer to reorder later than
a store to that same pointer, and boy, that's just not sound. In the
scenario constructed by the test we reorder this swap,
x = load32 X
y = load32 Y
store32 X y
store32 Y x
using schedule() (following Op argument data dependencies) into
y = load32 Y
store32 X y
x = load32 X
store32 Y x
which moves `x = load32 X` illegally past `store X y`.
We write `y` twice instead of swapping `x` and `y`.
It's not impossible to implement that extra reordering constraint: I
think it's easiest to think about by adding implicit use edges in
schedule() from stores to prior loads of the same pointer. But that'd
be a little complicated to implement, and doesn't handle aliasing at
all, so I decided to ponder on other approaches that handle a wider
range of programs or would have a simpler implementation to reason
about. I ended up walking through this rough chain of ideas:
0) reorder using only Op argument data dependencies (HEAD)
1) don't let load(ptr) pass store(ptr) (above)
2) don't let any load pass any store (allows aliasing)
3) don't reorder any Op that touches memory
4) don't reorder any Op, period.
This CL is 4). It's certainly the easiest and cheapest implementation.
It's not clear to me that we need this scheduling, and should we find we
really want it I'll come back and work back through the list until we
find something that meets our needs.
(Hoisting of uniforms is unaffected here.)
Change-Id: I7765b1d16202e0645b11295f7e30c5e09f2b7339
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/350256
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
The code didn't take into account that x and y might be different types.
(This bug was not actually harmful; type coercion allowed the code to
compile even with the wrong type. The float would be silently splatted
into a vec and the rest of the code would work as-is.)
Change-Id: Ib76bc733f76304e451ef9197421b4bc22e29e49c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/348888
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This actually exposed a latent bug: we don't support bool(1.23) or
bool(1) casts, but these are valid in GLSL:
https://www.khronos.org/opengl/wiki/Data_Type_(GLSL)#Conversion_constructors
"to bool: A value equal to 0 or 0.0 becomes false; anything else is
true."
Change-Id: Ia929a09914ffc96f081d0402d7bb05b5428f8db6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/349977
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: Ieb7698d357c9be05ca1f17de84215add54553f84
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/349065
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I2c39df532803d827d7cad876021f2ead81145f1d
Bug: skia:10902
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/349064
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Previously, the declaration didn't link back to function definition.
This makes the function appear to be undefined, which inhibits inlining
and also makes it difficult for us to validate the presence of a
definition for every called function.
Change-Id: I220ab502634cb3e1d337c23bac150af9aa6370b1
Bug: skia:10902
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/349063
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>
Previously, we did very little to distinguish between a built-in
intrinsic and a user-defined function whose name matches an intrinsic.
This could lead to all sorts of surprising outcomes, as our intrinsic-
rewriting code is able to make assumptions that might not hold true for
arbitrary user-defined functions.
Change-Id: I4180e0c5becdeb6a0a162534eaecfc90dda3392c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/349062
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: I6c0a6192a78ce60be60a71ed75350ca1bc256d57
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/348890
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I954af70f545a2258babd82af0d43d509201fdc59
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/348889
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I4798263318c504834f23900dbb3f5d167fd17e65
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/348887
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Bug: skia:10913
Change-Id: I430e5eb3fecb0f15775db03699819194d44271b6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/347958
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This code was not using typeName() to emit its types, inadvertently
generating Metal code containing the `half` type.
We didn't have any unit tests which synthesized a matrix-construct
helper with half types, so Matrices.sksl was cloned into two separate
test files--MatricesFloat and MatricesHalf. These should be equivalent
except for float vs half types.
Change-Id: I19ecea994b8bc45594bb3f69e596896a3bcefe4d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/348180
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
FrExp testing was moved to the intrinsic tests as part of
http://review.skia.org/341977, and the shared versions were removed from
sksl_tests.gni at that time.
Change-Id: Ife7f3622034d97a77b60d5a98c01f71630c161d6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/348183
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
In Metal, matrix *= matrix is not natively supported and needs to be
injected via a helper function. This helper function now properly
converts `halfNxM` types to `floatNxM` types (as Metal does not support
half types). It also returns the result by reference instead of by
value to avoid an unnecessary copy.
Matrices.sksl now includes tests for operators += -= *=. Previously we
did not have any coverage for `matrix *= matrix` at all.
Change-Id: I7dfe468ced67eaf7c2405960e8c5efe6f2acf9e4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/348178
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>
4x4 was dividing a matrix by a scalar - this isn't allowed, multiply by
the scalar's inverse instead.
The types in the signature were derived from type.name(), which wasn't
applying the half->float re-mapping.
Finally, use raw strings so the resulting shader code isn't all crammed
on one line.
Bug: skia:10913
Change-Id: Ie28373fc138445b8c195dbd37687e4ad4504e918
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/348177
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Previously, SKSL_INT was limited to an int32_t, so we couldn't
differentiate between -1 and 4294967295. We could paper over the
difference in some cases by relying on the expression's type, but this
was imperfect and left us unable to differentiate between an overflow
and valid results. SKSL_INT is now an int64_t; the code has been
updated to fix bugs that shook out as a result of the change.
This isn't a complete solution for overflow handling. There are still
lots of obvious places for improvement--e.g. constant folding can
easily overflow, and statements like `byte x = 1000;` are still
happily accepted.
Change-Id: I30d1f56b6f264543f3aa83046f43c2eb56d5fce4
Bug: skia:10932
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345173
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Bug: skia:10913
Change-Id: I59f5b0fb2d015f8543b4038c2c5b18ce24c194a8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/347956
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Previously, some types of overflow were detected, but most would assert
or silently generate invalid code. Now, the parser will properly report
an error if it encounters any integer that exceeds UINT_MAX or any float
that exceeds FLT_MAX.
This fixes test OverflowUintLiteral.sksl. Added a test for floats as
well, OverflowFloatLiteral.sksl.
OverflowIntLiteral.sksl does not fail yet, because its values are larger
than INT_MAX, not UINT_MAX. These are legal from the perspective of the
parser. This must be caught later at IR generation time.
Change-Id: Ia5a904d01427cdc9f2ab5f4174154418737835e6
Bug: skia:10932
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/347176
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This fix is overly conservative in some situations (identity conversions
among vectors with the same component type), but fixes errors in two
existing unit test cases.
Bug: skia:11116
Change-Id: If852f8591fb26817528fdc37191c49129e17d6b3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/347053
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This feature had devolved to just an assert, and one that isn't really
necessary - all of Ganesh is built to handle any child processor being
null. The next step is to remove nullable types entirely -- a large
amount of code.
Change-Id: I612a5867f8690400b405aa1f5c929e76cf5918fd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/347050
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
This CL updates `compareConstant` to fail gracefully instead of
aborting if the passed-in types don't match. This lets us call
`compareConstant` without checking types first.
Change-Id: Id2acdbdf700e64bcb24825cdad2c0e000992e8cb
Bug: oss-fuzz:28904
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/347038
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
These are GLSL-isms that weren't really implemented - each one was a
"generic" type that only resolved to a single underlying type. We've
got along just fine without them for years, so update our sample()
declarations to take the actual underlying type. (Note that we had
worked around this by declaring an integer version of sample where
necessary, so we can presumably keep doing that in the future).
Change-Id: I4c46a2fa0c1f19e6278298c8005a2760329e7abf
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/347040
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Nullable fragment processors still exist, but they're handled
transparently by sample() within C++, so there's no need for .fp files
to ever do these tests manually.
Change-Id: Idf2bc58505207560553066c0126a2a036c5d970b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/347039
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Opaque types can no longer be copied via assignment or construction, and
various restrictions originally applied to the "fragmentProcessor" type
have been extended to cover opaque types in general.
Change-Id: I55ab7aefd1e6ef277e56a9408b430e1de5ba12ca
Bug: skia:11027
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/346264
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 intrinsic was previously lacking a unit test, and wasn't actually
implemented in Metal or SPIR-V. Fortunately it's trivial to add.
Change-Id: I68bbdc58376b579c7f3f0ae5f49323b389c2b8c4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/346263
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>
Previously, a return statement inside a scoped Block would always result
in the return expression being assigned to a temporary variable instead
of replacing the function-call-expression directly. This was done
because there might be variables inside the Block; these would have
fallen out of scope when the expression is migrated to the call site,
resulting in an invalid expression.
We aren't actually examining the return expression so we don't know if
it uses variables from an inner scope at all. (Inspecting the return
expression for variable usage is certainly possible! But it's a fair
amount of code and complexity for a small payoff.)
However, we can very easily get most of the benefit here without paying
for the complexity. In this CL we now look for variable declarations
inside of scoped Blocks. If the code doesn't add any vardecls into
scoped Blocks, there's no risk of scope problems, and we don't need to
use a temp-var to store our return expressions. If any vardecls are
added, we go back to using a temp-var as before.
Change-Id: I4c81400dad2f33db06a1c18eb671ba2140232006
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/346499
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Also renamed $matH to $hmat, to match $hvec convention. Runtime effects
will only support square matrices (like ES2), so this lets us declare
intrinsics like matrixCompMult correctly (and differently) for public
vs. private usage.
Bug: skia:11093
Change-Id: I457d83e4c5e09f8e01e7b8acb116c39ff17e52c3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/346656
Auto-Submit: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Includes a handful of test cases to exercise the system
Change-Id: I98e73a8bca063f475d2ddb51778e395697392ddb
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/346637
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
We have a handful of tests that demonstrate this behavior indirectly,
but lacked a focused test.
Change-Id: I895cc4e3bebf30721ed649244e42bf170cc6ec06
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/346497
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 need to rescan after optimizing away expressions that might exist
in the CFG/definition map, since we are rebuilding them from scratch and
not just stripping off excess parts from them.
Change-Id: I843a2ea3fc38428e7c0bd0e2bf7a7d41101345e3
Bug: oss-fuzz:28794
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344972
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit 4129b6b65f.
Reason for revert: WASM breakage: https://task-driver.skia.org/td/UBRwnWYfbc5IwUWqtFMv
Original change's description:
> Revert "Reland "Reland "Reland "Revert "Initial land of SkSL DSL."""""
>
> This reverts commit 346dd53ac0.
>
> Change-Id: I93bb18438cc6c2ad43d058d6c3f95bcc65d0cea9
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/343916
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> Reviewed-by: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
# Not skipping CQ checks because this is a reland.
Change-Id: If05145cf9d9c51f4c76fe523f6050a670b5da669
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345169
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Because we use `continue` for flow control handling now, we can escape
from the middle of a switch statement. This wasn't possible when we used
`break`.
This unlocks some pretty stellar optimization opportunities if the
switch value can be determined at compile time; see BlendEnum for an
example.
Change-Id: Id29be92c343c10fd604683a80c5d5bd2bd070cb0
Bug: skia:11097
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345419
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
When we determine that a function only contains a single return
statement and it is at the top level (i.e. not inside any scopes),
there is no need to create a temporary variable and store the
result expression into a variable. Instead, we can directly replace
the function-call expression with the return-statement's expression.
Unlike my previous solution, this does not require variable
declarations to be rewritten. The no-scopes limitation makes it
slightly less effective in theory, but in practice we still get
almost all of the benefit. The no-scope limitation bites us on
structures like
@if (true) {
return x;
} else {
return y;
}
Which will optimize away the if, but leave the scope:
{
return x;
}
However, this is not a big deal; the biggest wins are single-line
helper functions like `guarded_divide` and `unpremul` which retain
the full benefit.
Change-Id: I7fbb725e65db021b9795c04c816819669815578f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345167
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
If we aren't wrapping the inlined function body in a loop, there's no
need to add a scopeless Block; we've already got one. This doesn't
affect the final output meaningfully--it just suppresses a newline--but
it's one fewer IRNode allocation.
Change-Id: Ib7b0014e908586d8acfcf6c23520873fad31d0b7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345163
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
do-while loops aren't compatible with GLSL ES2. For-loops which run
only one time should work exactly the same for our purposes. We expect
such a loop to be unrolled by every driver, so it shouldn't come at any
performance cost.
Change-Id: Ia8de5fcab8128c34da97eaeaf81f91ad1ac36ce4
Bug: skia:11097
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345159
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Fortunately, this had an existing opcode, so it was easy to add to our
intrinsics list, and the rest automatically worked.
Change-Id: Idcd5a2c46d6bf10c05c702faba4280a270c54929
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345398
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
We are still missing an implementation for Metal and SPIR-V, but at
least it's correct on GLSL now.
Change-Id: I5b365384eaefacb00faf6af7bda9b690cba00de5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345397
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>
Previously, multiple inliner passes in a row would each apply a
separate name mangling to variable names, so names like "_25_14_3_1_pos"
were not uncommon. This change demangles the name before re-mangling it,
so we would have just "_25_pos" instead.
It's not important, but it makes things easier to read.
Change-Id: I1257222dac2a68e337f431af230ce50730cedc9b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345116
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit e8e4aca955.
Reason for revert: can break ES2 for-loop rules
Original change's description:
> Declare all inlined variables at the topmost scope possible.
>
> By itself, this is uninteresting and even perhaps slightly
> counterproductive (as it separates vardecl from its initializer,
> increasing LOC). However, this enables a followup CL
> (http://review.skia.org/344665) which allows single-return functions to
> be inlined without the creation of a temporary variable at all. This
> applies to the majority of fragment processors in a typical Ganesh
> hierarchy. This change will greatly reduce the number of inliner-created
> temporary copies when compiling a typical tree of FPs.
>
> Change-Id: I03423a13cf35050637dabace4a32973a08a4ed0a
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344764
> 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: Ica01d6906bcb9cef1f49d22dda714fc9cbfa3885
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345121
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This reverts commit 345d72124d.
Reason for revert: can break ES2 for-loop rules
Original change's description:
> Eliminate inliner temporary variables for functions with a single exit.
>
> When we determine that a function only contains a single return
> statement, there is no need to create a temporary variable and store the
> result expression into a variable. Instead, we can directly replace the
> function-call expression with the return-statement's expression.
>
> This dramatically simplifies the final optimized output from chains of
> very simple inlined functions, which is a very common pattern for trees
> of Skia fragment processors.
>
> Change-Id: I6789064a321daf43db2e1cef4915f25ed74d6131
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344665
> Commit-Queue: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I60845f22159605a06047b030e2686a769121a35a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345120
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This reverts commit 67d2d73d06.
Reason for revert: Triggers errors in some shaders.
Original change's description:
> Fix incorrect 'unreachable code' error in SkSL
>
> This trades one error for another (a potential for incorrect use of
> unassigned variables). False-positives for unassigned variables are
> straightforward to workaround (and produce code that still looks
> reasonable). Working around unreachable code errors is tricky, and
> likely to produce non-idiomatic code. This change also makes the data
> flow analysis of all loop constructs more similar - for loops were
> behaving very differently from while loops.
>
> Note that this effectively a revert of:
> https://skia-review.googlesource.com/c/skia/+/18121/
>
> Change-Id: Ib85d90b22cac8addfb106459c0a5f5616a89c3eb
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344957
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
> Commit-Queue: Brian Osman <brianosman@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I84b93cc7f9309446dcc0e949e90908df31a1ff9c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345119
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
When we determine that a function only contains a single return
statement, there is no need to create a temporary variable and store the
result expression into a variable. Instead, we can directly replace the
function-call expression with the return-statement's expression.
This dramatically simplifies the final optimized output from chains of
very simple inlined functions, which is a very common pattern for trees
of Skia fragment processors.
Change-Id: I6789064a321daf43db2e1cef4915f25ed74d6131
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344665
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
By itself, this is uninteresting and even perhaps slightly
counterproductive (as it separates vardecl from its initializer,
increasing LOC). However, this enables a followup CL
(http://review.skia.org/344665) which allows single-return functions to
be inlined without the creation of a temporary variable at all. This
applies to the majority of fragment processors in a typical Ganesh
hierarchy. This change will greatly reduce the number of inliner-created
temporary copies when compiling a typical tree of FPs.
Change-Id: I03423a13cf35050637dabace4a32973a08a4ed0a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344764
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This trades one error for another (a potential for incorrect use of
unassigned variables). False-positives for unassigned variables are
straightforward to workaround (and produce code that still looks
reasonable). Working around unreachable code errors is tricky, and
likely to produce non-idiomatic code. This change also makes the data
flow analysis of all loop constructs more similar - for loops were
behaving very differently from while loops.
Note that this effectively a revert of:
https://skia-review.googlesource.com/c/skia/+/18121/
Change-Id: Ib85d90b22cac8addfb106459c0a5f5616a89c3eb
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344957
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
The additional scopes were harmless, but didn't really add any value.
Originally they were used to tightly scope inlined variables, but we now
mangle inlined variable names.
Change-Id: I7b35e737598036c0b6d3d9f71cbcd4a53d609ce9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344757
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
All fragment processors now use explicit returns; sk_OutColor no longer
exists at all.
Change-Id: Ic5cf566a916c1d616edcc56ba84b6780776f8515
Bug: skia:10549
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344300
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I41a5aea7b01efe8901498621197b9a5ff0f4fe5f
Bug: skia:10549
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344656
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
findMSB has one special trick that Metal doesn't naturally have an
equivalent for, specifically in its treatment of negative numbers.
findMSB searches negative numbers for a zero bit, not a one bit!
We emulate this behavior in Metal using select(n, ~n, n<0).
Change-Id: I861c6b8fb3dc5427643cd8c68a39a53f1959bff3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/343996
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, coercion between a signed type and an unsigned type was
treated as "no cost" because these types shared the exact same priority.
This meant that we couldn't choose the proper overload with function
calls that only differed in signed-ness, like:
void fn(int4 x);
void fn(uint4 x);
So we would always choose the int4 version since we encountered it
first. Now, we can choose the correct overload; signed types now have
a slightly elevated priority over unsigned types, allowing coercion
costs to work normally.
Also added some comments to `determineFinalTypes` while trying to see
if that needed some improvements as well, but this turned out to be
a red herring--it didn't need any functional changes.
Change-Id: I334debae9ad0e9b290109658d2fde8f6526770a2
Bug: skia:10999
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344017
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: I1a96060b2e52cddb50948a48520aab30bd097bbd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/343577
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This reverts commit b37a693254.
Reason for revert: Breaking Flutter roll
Original change's description:
> Revert "Reland "Reland "Revert "Initial land of SkSL DSL.""""
>
> This reverts commit 6b07e0eb49.
>
> Change-Id: Ic01f31edf55b2d1a7533e0e8ed33b39b4846d937
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/343106
> Reviewed-by: John Stiles <johnstiles@google.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> Auto-Submit: Ethan Nicholas <ethannicholas@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
# Not skipping CQ checks because this is a reland.
Change-Id: I3373f186f4d0531bc8ab1e4392c512608389734f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/343518
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Change-Id: If29ee048d359d0ccd7b0ab708f54d40746b92386
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/343423
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: Id38a3d04fcb1904a4c666d92087b4fe14bd03a27
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/343110
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit 6b07e0eb49.
Change-Id: Ic01f31edf55b2d1a7533e0e8ed33b39b4846d937
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/343106
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: Ethan Nicholas <ethannicholas@google.com>
Change-Id: I8eae896f5a859b59a1ba0b29dc95d5aa070c12ae
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/343102
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I4a483c455c9a12c92b717a0c2713d32ab44dcd6f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/343099
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Separating this out from landing SkSL DSL, as it's causing unrelated
test churn as the DSL code gets submitted and reverted.
Change-Id: I0d3ade4ca8d1b0c302ccc494f0192a4f5ae67086
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/343109
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I0e289a136678fe863445f739f162a718c341977f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/343104
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Bug: skia:11072
Change-Id: Ic24e40bfea5bf1d2d14c0f681632228a5ecc7104
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342929
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Auto-Submit: Brian Osman <brianosman@google.com>
This reverts commit 52e5850065.
Reason for revert: Failing on Build-Debian9-Clang-arm-Release-Flutter_Android_Docker: https://logs.chromium.org/logs/skia/5066a8ed31374c11/+/steps/Run_build_script_in_Docker/0/stdout
Original change's description:
> Revert "Reland "Revert "Initial land of SkSL DSL."""
>
> This reverts commit 53f69f1539.
>
> Change-Id: I374b016c8a08d83c99cbab800f30b882244b87f1
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342919
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
> Auto-Submit: Ethan Nicholas <ethannicholas@google.com>
> Reviewed-by: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
# Not skipping CQ checks because this is a reland.
Change-Id: Ia04ee404478314b3ae034e0a7740ef667364b2f8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/343100
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
The existing code didn't work properly with half types since the $mat
type encompassed both halfNxM and floatNxM. This was fixed by splitting
the half types out of $mat into a separate $matH generic.
Unit tests now compile properly for GLSL, but generate errors in SPIR-V
and generate Metal code which attempts to call a non-existent intrinsic.
Change-Id: I2fff10f0dd7f00534bf6b1d5b13354543694194e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342926
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit 53f69f1539.
Change-Id: I374b016c8a08d83c99cbab800f30b882244b87f1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342919
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Change-Id: I7af94d89d349b67b2c070179324fcad7b62e0d1e
Bug: skia:11071
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342758
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I1d5a056e08ba6e67016e45c52518da6074a62c8f
Bug: skia:11071
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342759
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
floatBitsToUint was missing from our intrinsic list entirely, and
u?intBitsToFloat were misspelled.
These intrinsics aren't implemented in SPIR-V or Metal either, but that
will be handled in followup CLs.
Change-Id: Iaf9b9d5a2e46e25d41eef71903fad8bd1c177d4e
Bug: skia:11071
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342757
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit a3b8ac76e5.
Reason for revert: Need to revert again, red tree.
Original change's description:
> Revert "Revert "Initial land of SkSL DSL.""
>
> This reverts commit dd213e9d46.
>
> Change-Id: I43be020dd1b07dc13862150a9d95493f8c48b3b1
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342622
> Auto-Submit: Ethan Nicholas <ethannicholas@google.com>
> Reviewed-by: John Stiles <johnstiles@google.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
# Not skipping CQ checks because this is a reland.
No-Presubmit: true
No-Try: true
Change-Id: I8e967ef8ecb7f01dc578d38264e2600b04e9b62d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342917
Reviewed-by: Jorge Betancourt <jmbetancourt@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Change-Id: I887e700a7bf11bf2d5359c9721798f72f00e53f3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342756
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit dd213e9d46.
Change-Id: I43be020dd1b07dc13862150a9d95493f8c48b3b1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342622
Auto-Submit: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Change-Id: I674d758c11071582e9fbedcda5596c540bfb5f71
Bug: skia:11054
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342558
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This does not give us 100% coverage of intrinsics yet, but it is a
pretty good start.
Change-Id: I97d49324db1afd9f2975c2eeafbacdead710d4aa
Bug: skia:11054
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/341977
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
