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>
Like getIVecComponent or getFVecComponent, this retrieves the n'th
element of a Boolean compile-time constant vector. This will be used in
followup CLs.
Change-Id: Ib41c9c89cb773251e4c0d6cdcaea0437d8074e48
Bug: skia:11141
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/350918
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This will be used in a followup CL to implement getBVecComponent. At
present it's not called.
Change-Id: Idd6f18314d0835af3946ea7458e6650384f505ea
Bug: skia:11141
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/350703
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Previously ExternalValues were flexible, and could be used as raw values
(with the ability to chain access via dot notation), or they could be
callable. The only non-test use-case has been for functions (in
particles) for a long time. With the push towards SkVM, limiting
ourselves to this interface simplifies things: external functions are
basically custom intrinsics (and with the SkVM backend, they'll just get
access to the builder, and be able to do any math, as well as
loads/stores, etc).
By narrowing the feature set, we can rename everything to reflect that,
and it's overall clearer (the SkSL types now mirror FunctionReference
and FunctionCall directly, particularly in how they're handled by the
CFG and inliner).
Change-Id: Ib5dd34158ff85aae6c297408a92ace5485a08190
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/350704
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Continue to test everything with the ByteCode interpreter, and run most
tests with the new SkSL-to-SkVM utilities, as well. A few tests rely on
features that aren't yet implemented (function calls, looping), and some
of the bespoke tests (that don't use the test() helpers) use even more
exotic features that need to be implemented or disallowed in the IR
generator. This is getting us closer to not needing ByteCode at all,
though.
Refactored a bunch of the helper code to reduce copy-paste among the
many different 'test' functions.
Change-Id: I138d4a24266f2d862742245c5ee895d86c01018e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/350560
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Mike Klein <mtklein@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>
This CL adds additional variants of some IRGenerator functions to enable
them to be more easily called from upcoming DSL code. This should not
change functionality (other than a slightly different error message in
one case).
Change-Id: Ic727c194fca5111d4283007f4ec23653598a853b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/350017
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Auto-Submit: Ethan Nicholas <ethannicholas@google.com>
IntLiteral and FloatLiteral each had a pair of public constructors: one
which takes a Type (what type of Int/Float is this?), and another
constructor which takes a Context (assume the generic Float/Int type).
BoolLiteral had a public Context-based constructor, but its Type-based
constructor was private. The Type-based constructor is now public to
match its siblings. This allows us to create Booleans without needing
a Context&, if we have an fBool_Type, and fixes make_unique (which
doesn't like private constructors).
Change-Id: Iffb4980947a73b3a89aa1452cc90dce63620bd67
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/350636
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: 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>
Fixes terminology inside the generator to correctly use 'arguments'
rather than 'parameters'. Retains the caller-supplied argument span, so
that generateCode can push changes to 'out' and 'inout' parameters back.
Adds a useful helper function for getting a named function from a
Program.
All of this is used more extensively in upcoming CLs that migrate
interpreter unit tests (and particles) to SkVM.
Change-Id: I9d9aef4b7c4daf93d822e22f813162f4ed2385be
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/350023
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Brian Osman <brianosman@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>
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>
Previously, we had three separate implementations of intrinsics:
- "Special" intrinsics (the common case)
- "Metal" intrinsics (the GLSL bvec comparison functions)
- A hardcoded list of if-then workarounds in `writeFunctionCall`
There doesn't seem to be any particular rhyme or reason for why things
were organized in this way. These all been merged into the "Special"
intrinsic handling.
Change-Id: I10e4eed00a2dd2c7cb1f79a4b96460fc43a68ebb
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/349059
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@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>
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>
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>
These are part of the GLSL ES 1.0 spec.
Bug: skia:10913
Change-Id: I61bfcadc92209fb3a6aa07b3d6ad579460ad2ecd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/347049
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This was only used for nullable fragment processors, which are gone.
Change-Id: I1ea805c683995367a7525b787c9113ae6d2d0ae0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/347051
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@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>
'int' is the only integral type that exists in GLSL ES 1.0 (and it's not
really guaranteed to be an integer). This enforces the same restriction
on runtime effects - no unsigned integers, and no short or byte types.
Bug: skia:11093
Change-Id: I938f1e0e125dc8347507f428b46b51c66033c752
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/347046
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
More GLSL-isms that weren't used, and weren't really ready for use (most
were defined as "Other", not "Sampler"). If/when we need these, it's
easy to add them back. In the meantime, we should have a simpler system
for reserving keywords that doesn't pollute the type system.
Bug: skia:11115
Change-Id: I436c1e4de6e6b92ff14fc99ed1d47e0c5d1e3aff
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/347045
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Missed these when I moved the float versions earlier. Non-square
matrices don't exist in our minimum spec (GLSL ES 1.0).
Bug: skia:11093
Change-Id: I09b3ab71199bc70d9b54302c14b93bc3f3dec2d0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/347042
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Auto-Submit: 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>
These were unused, and I don't think they were correctly defined, given
the inconsistency in naming and construction. If we do end up needing
something like this (a variant type for returns from sample(), we can
re-add them correctly).
Bug: skia:8863
Change-Id: I32a9487ba0e247f67696947babbfd6b64f0a047c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/347037
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>
Change-Id: I2d396907387de4a5f3407d81efb9d2cd80e430d1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/346265
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@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>
This is slightly simpler than having three separate overloads, and
provides the same results.
Change-Id: Icc7a749fd642f6d6a9e69b769494c566569ea8f4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/346262
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>
Updated Pair type in SkTHashMap to derive from std::pair to fix C++14
issues with structured bindings.
Original change's description:
> Add support for range-based for loops to SkTHashSet/Map.
>
> This allows loops over SkTHashes to break in the middle, and also
> removes the need to use lambda captures to bring variables inside the
> loop's scope.
>
> Change-Id: Ief55d776b2c57a44b24cfe1c94493a5d514791c8
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/346496
> Reviewed-by: Mike Klein <mtklein@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
Change-Id: I2ac5b2c59e70ed0ec3b42b32e7994d6bcdf56b40
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/346502
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
I checked that this prevents diffs on my M1 Mac.
Without this we'd see FMAs.
Change-Id: I24fa2e301cde556de556332b472399b97e6a08fe
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/346676
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Mike Klein <mtklein@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>
These don't exist in our minimum spec (GLSL ES 1.0)
Bug: skia:11093
Change-Id: Ia2d871199fff2a98dcd517c1eebe46decb0c2dfb
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/346657
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@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>
Change-Id: Ida9d81a8d4e530cb3055f01418a2ad2893ae86db
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345422
Reviewed-by: Mike Klein <mtklein@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This reverts commit 5d00e15625.
Reason for revert: tree breakage on Chromebook standard lib
Original change's description:
> Add support for range-based for loops to SkTHashSet/Map.
>
> This allows loops over SkTHashes to break in the middle, and also
> removes the need to use lambda captures to bring variables inside the
> loop's scope.
>
> Change-Id: Ief55d776b2c57a44b24cfe1c94493a5d514791c8
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/346496
> Reviewed-by: Mike Klein <mtklein@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
TBR=mtklein@google.com,johnstiles@google.com
Change-Id: I165872ac41f66f3b3255cf8970626392e5283412
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/346500
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This allows loops over SkTHashes to break in the middle, and also
removes the need to use lambda captures to bring variables inside the
loop's scope.
Change-Id: Ief55d776b2c57a44b24cfe1c94493a5d514791c8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/346496
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: John Stiles <johnstiles@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 was added to workaround a bug in D3D (w/SPIRV-cross). It's now
handled correctly in SPIRV-cross itself, so this is unused.
Change-Id: Ic411ee2219cf370bcebff18ae18689d16da90866
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344966
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This maps to usage better, and makes some code simpler to understand.
Note that there is still a PipelineStage *back-end*, which is specific
to the runtime-effect FP. A kRuntimeEffect_Kind program can be used to
generate a PipelineStage (for the GPU backend), or an skvm program (for
the CPU backend).
Change-Id: Id3f535db93a239726c595225aafe9467f0d19817
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344969
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Nulls aren't generally expected here, but this is a debugging utility
and in practice it's more useful to print nulls cleanly than to crash
while trying to dump out a CFG.
Change-Id: I405c1e51875655acaf4f9689b47e3e6a6b8751df
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344967
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>
Rather than return a complex ternary expression, we now assign the
complex expression into a variable and then return that variable.
This works around a bug in the Adreno driver where it fails to parse
"(complex-expression).swizzle" and errors out with "Cannot offset into
the vector." Instead, we now store the complex expression into a
variable and return that variable. This changes the final assembled
code into a two-parter, "float2 v = (complex-expression);" followed by
"v.swizzle". This is easier for the Adreno driver to swallow and works
around the error seamlessly.
Example Adreno failure: http://screen/Bkx5zRvaMEbuCTh
Change-Id: Ib58abd404d8a1d7ce664acd38300ef75d0296363
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345133
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@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>
Instead, add them to the shared element list (like functions and
variables).
Bug: skia:10905
Change-Id: Ib52b4c5eaca9a3023532c5f7021d0ac263ab64ac
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345476
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@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>
Still TODO:
- Function calls. Can do these by inlining everything (recursively).
- Additional flow control: ES2 for loops can be supported (via
unrolling), and switch() can be implemented. (Was never done in
ByteCode).
- Uniforms and params have been set up to work very generically (caller
just supplies IDs, the generator collates everything into a master
working list). Builtins should work the same way (caller supplies a
map of builtin ID -> skvm::Val[]?), so that we don't need a special
case for device coord.
- Figure out integer type policy. Today, it's a mix of only supporting
signed integers, and just treating all integers as signed, even if
they're not.
- Now that defining intrinsics is *much* simpler, stop defining so many
of them in sksl_public.sksl, and just implement them here.
Change-Id: Id9771444ce54ccf8e6e408c44ebb3780c1170435
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/341980
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Mike Klein <mtklein@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>
In some cases, it's okay if we can't find a path to a CFG block. Two
such cases are:
- The starting block. This is reached implicitly.
- For-loop increment expressions. These are sometimes required due to
GLSL ES2 restrictions, even if they can't actually be reached.
Change-Id: I626c424361a7339d4fb1ab0809f60df3f2f72173
Bug: skia:11097
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345162
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Bug: skia:11095
Change-Id: Icd69df40675e5ecde5004e04a7dcd78eedf8343c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344765
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
This will be utilized in followup CLs.
Change-Id: I591cff5b8942a5be57660a4bd161517880fbb8a4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345157
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This fixes an issue where for loops with no test created invalid code,
and improves tracking and adds asserts to keep us honest in the future.
Change-Id: Ie2212b237948bf245ca4a2918bc6b018540646c3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345128
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>
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>
Allowing this in runtime effects lets people break our contracts around
SkShaders on the color side of the paint altering coverage, etc.
Bug: skia:11085
Change-Id: I1ec8e71581c8d50f681cb0ca6ca8416375b3f43f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344760
Auto-Submit: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@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>
This reverts commit b476981949.
Reason for revert: https://logs.chromium.org/logs/skia/507fb79e5cf79811/+/steps/dm/0/stdout
Original change's description:
> Add integer relational ops to sksl_public.
>
> These aren't implemented yet in the software rasterizer, but work
> properly on GPU and serve as a good end-to-end test for skia:10999.
>
> Change-Id: I1dac66cd5762ebde828e819c1b6890016599672e
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344036
> Auto-Submit: John Stiles <johnstiles@google.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: Ic9ace24c3f0ea764b2720bffa50792a136d452ef
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344558
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
These aren't implemented yet in the software rasterizer, but work
properly on GPU and serve as a good end-to-end test for skia:10999.
Change-Id: I1dac66cd5762ebde828e819c1b6890016599672e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/344036
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@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 makes it much easier to understand what sorts of types we are
creating.
This has some minor repercussions for the SPIR-V code generator, which
actually created temporary Types on the stack occasionally, but these
were simple to fix.
Change-Id: I1ca43cdef0445d2b9789a435221dce50b03d954a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/343517
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: 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>
I didn't notice the intrinsic map the first time around; this is better
than a bunch of string comparisons in a row.
This doesn't change the results, just changes how we get there.
Change-Id: Iab250b71b6bccc4ff0e0cce6f23658985d48ac59
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/343097
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>
This intrinsic multiplies the vertex against the fixed-function
gl_ModelViewProjectionMatrix matrix, which is not a thing in Skia.
Change-Id: Ie361d9d631762b1397ee7be062bda317b415c7c9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342923
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: 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>
We now insert helper functions which defer the assignment of out-
parameters back into their original variables to the end of the
function call. This allows us to match the semantics listed the GLSL
spec in section 6.1.1:
"All arguments are evaluated at call time, exactly once, in order, from
left to right. [...] Evaluation of an out parameter results in an
l-value that is used to copy out a value when the function returns.
Evaluation of an inout parameter results in both a value and an l-value;
the value is copied to the formal parameter at call time and the lvalue
is used to copy out a value when the function returns."
This technique also allows us to support swizzled out-parameters in
Metal, by reading the swizzle into a temp variable, calling the original
function, and then re-assigning the result back into the original
swizzle expression.
At present, we don't deduplicate these helper functions, so in theory
there could be a fair amount of redundant code generated if a function
with out parameters is called many times in a row. The cost of properly
deduplicating them is probably larger than the benefit in the 99% case.
Change-Id: Iefc922ac9e2b24ef2ff1e9dacb17a735a75ec8ea
Bug: skia:10855, skia:11052
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/341162
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit 6e599511d4.
Reason for revert: Breaking bots: https://logs.chromium.org/logs/skia/5061fbd134144011/+/steps/dm/0/stdout
Original change's description:
> Initial land of SkSL DSL.
>
> This is not 100% complete: it lacks support for several kinds of nodes
> and supports only a bare handful of builtin functions, but it
> demonstrates the core functionality and it should be relatively
> straightforward to fill in the missing pieces.
>
> Change-Id: I3058089338e20eebc3da18ac5571801abcaab564
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/331177
> Reviewed-by: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: Iee77e5322a0b1efb0f3718ec1f5976a4d4e7323a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342620
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
This is not 100% complete: it lacks support for several kinds of nodes
and supports only a bare handful of builtin functions, but it
demonstrates the core functionality and it should be relatively
straightforward to fill in the missing pieces.
Change-Id: I3058089338e20eebc3da18ac5571801abcaab564
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/331177
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Change-Id: Iac8096f6c225258b430858bad90199ec00b93b6c
Bug: skia:10998
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342304
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, `equal(a, b).x` would emit `a == b.x`.
Change-Id: I311116f45fb275b158a948a0fd165d97688ce8ff
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/341978
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
The code which detected boolean binary expressions did not check for
vector boolean types.
Change-Id: Ice12908e0f14c77dfddcde3f938e6fe1df7c57bd
Bug: skia:11060
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342302
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>
Also fixes some additional style mishaps in class method names.
Change-Id: I49e7ac1aa91d84fef5fbc636552f040a2cb58c78
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/341466
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I6a59fcd33d78a45a08762bfd0b62feea82ae5923
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342301
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: I2c958b7aca972b7eec07e10d6c8af95fa53e761a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342117
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
A missing-intrinsic error is perfectly recoverable and shouldn't cause a
call to SK_ABORT.
Change-Id: I29db652770f4a091a344a294a8210dfd87567a45
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/342096
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: I4a443793d5f0fb6dac4abef07ed2ea33fff3a14a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/341721
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>
Our optimizer ignores index expressions, but has a few simplifications
that it can perform on swizzles. (Added extra code to SwizzleByIndex
which demonstrates this.)
Change-Id: If3c85a0456d98749008d796e422944b602ee6933
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/341460
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Short/ushort types are valid as-is and don't need to be coerced to int.
Change-Id: I41d6a537094e0c3f968e47926f541e0f6a3f92b4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/341459
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: Ib9117dbd1bcd2c3581fba02416d9eabda1dfc6dd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/341458
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This mirrors other "AutoXxxxxxx" classes used in SkSL to push and pop
temporary changes into member variables, such as AutoSymbolTable or
AutoLoopLevel.
Metal and Pipeline code generators were updated to use this class.
SkSL was left as-is for now; it modifies fOut more extensively than the
others and will need special care.
Change-Id: Icf505b9b55e3458de349e35e3b812dd005f9afed
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/341457
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This sort of error would be detected by most backend compilers. This
case was also detected by the bytecode generator. It's easy for us to do
a similar check during SkSL IR generation and report the error sooner.
Also, `convertIndex` had migrated a few hundred lines away from
`convertIndexExpression`, so I moved it back to live next to its parent.
Change-Id: I715d3abf42581782b55ba60df30d0296355667d4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/341377
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
We will need to emit a helper function to work around this case, as
GLSL supports swizzled out params, but Metal does not. In this CL, we
do not yet synthesize the helper function, but we annotate the code with
a comment indicating affected calls. (Of course, this will be replaced
with a helper function in a followup CL)
Even detecting a swizzle is actually an interesting problem, because
index expressions are sometimes actually swizzles, depending on the type
of the base expression. Also, the index or swizzle might be nested in
several other valid assignable expressions.
Change-Id: I8c74f9a7daec08eff1f32387f8b6b96851c1bd6e
Bug: skia:10855
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/341057
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Pointers require decorating the variable with a * to read back the
value, which the code generator did not properly handle. There was a
special case to add the * but it only supported assignment into the
variable, not reading back. References require no special decoration.
This change fixes compile errors in Functions.sksl with the "bar"
function. (This test marks `x` as an inout but never actually mutates
it.) It also allows us to remove a special-case workaround for `frexp`,
an intrinsic function which uses a reference for its out-parameter.
Additionally, this CL adds a non-inlining copy of "OutParams.sksl" to
the Metal test directory, as most of our tests which use out-parameters
end up inlining all the code, which hides these sorts of bugs.
Change-Id: I31c4db04f6b512b4cd4fe65b3347b82bdbf039cd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/341000
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Previously, we would emit an invalid [[buffer(-1)]] annotation on the
block, causing the Metal compilation to fail.
Change-Id: I68b2439c05db3163686e84c5dcc9a5c43870ff67
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/340761
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
These were added by a certain new team member who hadn't internalized
all the Skia style rules yet.
Change-Id: If8c53045428c61efb7a09c573885b17cb2ab360e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/340999
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>
It's not legal to use identifiers like "int" or "sampler" to name your
variables (or enums, or structs, etc.). SkSL will now report this as an
error instead of relying on the driver to catch this.
(Note that in some contexts, it might be legal by the spec to reuse a
name that you introduced yourself, depending on the scope. In practice,
this confuses Apple GLSL, so we shouldn't support it anyway.)
This caught several existing places in our code where we used the name
"sampler." These were never exposed to the driver (they were intrinsics
that we would replace during compilation) so they were harmless before.
Change-Id: Ia6dcfca8c500d02e1eb5f9427bed8727e114dfc2
Bug: skia:11036
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/340758
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
SkSL::Type now asserts if you try to create a multi-dimensional array;
various looping/recursing constructs that no longer need to loop or
recurse were updated.
Change-Id: I191b4a032ddc6e7759cebc8b41c536cfaaf1b626
Bug: skia:11026
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/340759
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
These methods improve readability for simple, commonly-performed type
checks.
We already had a (very rarely-used) helper function `isArrayed` which
was only applicable to texture and sampler types. To avoid potential
confusion, this has been renamed to `isArrayedTexture`.
Change-Id: Ibec9d872ff3b415964b842c96ddc1b5b271ac883
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/340720
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Just noticed one more spot in the code where an array type was being
hand-assembled.
Change-Id: I3c9d931caee3dc8e03b3eb016af5fa0a36064d57
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/340660
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
There's no need to pass in an array of multiple dimensions when only one
dimension is supported by the language.
Change-Id: Id170e96e1c0e8f83a79a85e4a737792677044150
Bug: skia:11026
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/340659
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Previously, our AST structures would include a "sizeCount" for arrays,
which indicated the number of AST nodes associated with array
dimensions. Since GLSL only supports a single array dimension, this
field has been replaced with "isArray," a boolean indicating whether we
have a single AST node for array size. This allowed many array-size
based looping constructs to be replaced with simpler non-looping
equivalents.
This change flushed out a few places where the parser was not actually
enforcing its promised maximum array-dimensionality.
Also found some duplicated code in variable-declaration parsing,
related to parsing array-sizes and initializer expressions. This has
been de-duplicated by using a lambda. (This change was likely why this
CL was not net-negative for LOC, but it's simpler and cleaner.)
Change-Id: I7abed732d3a296edf02c0ec9813fceb5aae4a9a0
Bug: skia:11026
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/340656
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Maintaining an array of Expression-based sizes is not necessary as GLSL
only supports a single dimension, and doesn't allow any expression other
than a constant integer or nothing (meaning "unsized").
Change-Id: Id58404c5c8d48786e02585d2a6391b2f3e5393e8
Bug: skia:11026
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/340456
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Maintaining an array of Expression-based sizes is not necessary as GLSL
only supports a single dimension, and doesn't allow any expression other
than a constant integer or nothing (meaning "unsized").
Change-Id: I01b5f88b94234a27e694aa2fc087f9d5f01b99c5
Bug: skia:11026
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/340341
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
GLSL only allows one-dimensional arrays. This CL lowers SkSL's array
dimensionality limit from eight to one, and fixes all the tests that
this breaks. The rest of the code still technically supports
arbitrarily-deep array dimensionality; there are many opportunities for
code cleanup and simplification in followup CLs.
Change-Id: I0fc31e4626649ec69d40c5f5597b3924de298df0
Bug: skia:11026
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/340339
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This is illegal in older versions of GLSL and in Metal. We now fail at
SkSL compilation time and properly report the error.
Change-Id: I6ddaeabff5386a1ed6ca3eb8703a6035476ec77a
Bug: skia:11021
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339298
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This CL fixes cases where array dimensions could be placed on the type
instead of the variable (`float[2] x` instead of `float x[2]`). It also
reports errors in cases where arrays aren't syntactically valid in
Metal, rather than emitting unusable Metal code. (Some of these cases
are actually invalid GLSL as well! But those fixes are coming in
followup CLs.)
Change-Id: I22279127c8a9aa2f22bf5ea3d225e563c2e254f2
Bug: skia:10926, skia:10760, skia:10761
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/340137
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
The proper approach for creating multi-dimensional array types is
complicated, so I added a function in SymbolTable which does it the
right way (addArrayDimensions). I found all the places in SkSL which
created arrays from base types and size arrays, and refactored them to
call addArrayDimensions instead of doing it manually.
I believe that this approach fixes a bunch of minor issues with multi-
dimensional array types; some are visible in the current codegen output,
and others are latent bugs. e.g. in some instances, a Variable's type()
was silently holding flipped array dimensions, but this never led to
a visible bug because we ended up using the VarDeclaration's baseType()
plus sizes() everywhere that the type was used. (In particular, this
caused debugging headaches in http://review.skia.org/340137 where I'd
use a Variable's type and suddenly its array dimensions would be wrong.)
Change-Id: Idd6a86aa5d1dce8918d02a53bcc2f7d7886e3ac5
Bug: skia:11016, skia:10924
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339860
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reland fixes link errors in nogpu builds
This reverts commit 5fa45548b4.
Change-Id: I45e0509d0476dde3a7088c1ed66ab0118894b31e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/340037
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This reverts commit 68da339a11.
Reason for revert: Breaking Android roll
Original change's description:
> Add ByteCode output to skslc
>
> Change-Id: I447f56a3ef464ef9a3cfc644f6ef4e4ab4e08a62
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339498
> Commit-Queue: Brian Osman <brianosman@google.com>
> Reviewed-by: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,johnstiles@google.com
Change-Id: Ie02d03dacc3b5ea33538d11dbb1241b8fe31fd86
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/340036
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
The Metal return type from main() diverges from the SkSL source, so we
patch it in the Metal code generator. This CL improves the patching
process in multiple ways:
- A `return` statement from a fragment processor main() is rewritten to:
return *_out;
- A `return` statement from a vertex processor main() is rewritten to:
return (_out->sk_Position.y = -_out->sk_Position.y, *_out);
- We avoid emitting a duplicate `return *_out;` statement if we can
determine that main() already ends in a return statement. This is
harmless either way so it doesn't necessarily catch everything. (e.g.
it doesn't detect an if/else which returns at the end of both blocks.)
Also added a unit test which returns from the middle of a vertex shader,
since we didn't test this anywhere and we need to verify that
sk_Position.y will be negated. (This didn't work properly before.)
Change-Id: I14cf18375894fc712fa6c6466df3888ebaeba7c8
Bug: skia:10903
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339636
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I447f56a3ef464ef9a3cfc644f6ef4e4ab4e08a62
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339498
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Previously, this would generate invalid code such as `[[user(locn-1)]]`.
We now generate a more-useful error at SkSL compilation time.
Change-Id: Ifbe335ec6d4abcbdfe89b892ba51063c94d22b11
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339397
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
GLSL only supports arrays of samplers in very limited ways; they aren't
supported at all by SkSL. We now detect arrays of opaque objects and
reject the code.
We have several paths through the IR generator that create and process
array types; the unit test covers global and local variables, and array
on the type versus array on the variable.
Change-Id: I5b45e88e31cf4005723c3bf35561622d65321f7b
Bug: skia:11008
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339317
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Upcoming CLs are going to add more thread_locals to SkSL, so it makes
sense to bake this test into a convenient define.
Change-Id: I5c878b16ecc0cd6f5dfeab37d16734cb9fd270bd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339717
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>
Add missing "not" intrinsic to SPIR-V, and several relational/logical
opcodes to runtime effect's skvm converter.
Bug: skia:10913
Change-Id: Ic349d491d980d0018134801260073414485f9059
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339316
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Constructors such as `float[2](0, 0)` add a type to the symbol table;
this type needs to be copied into the new symbol table if the
constructor is cloned by the inliner.
Change-Id: Ifa8d2dec87103c6223ce493e2201a904c14c2137
Bug: oss-fuzz:28050
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339168
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
We had special-case logic for copying array types from one symbol table
to another while inlining, but it only considered single-dimensional
array types. Each added dimension in an array has its own type, and
each type needs to be copied.
This bug could be triggered by compiling shared/Functions.sksl with
ASAN enabled.
Change-Id: Ib99d1e3f44b5530c271a97f374ee1d6d5ecf295c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339167
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
SPIR-V previously didn't know what to think when it encountered a Type
with a typeKind of kEnum, and would abort. These are now treated as
32-bit signed integers.
Metal previously emitted the SkSL enum typename, which is meaningless to
Metal since we do not emit the enum itself anywhere. Metal now emits
"int" for an enum-typed variable.
(GLSL already correctly emits "int" for enum types.)
Change-Id: I05975a2a399f9c4a22c00c90be0dccacd99d793b
Bug: skia:11003
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338856
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This CL addresses the root cause of the fuzzer issue, by checking for
LayoutIsSupported before getting the MemoryLayout of a type. However,
this array ought to be detected as an error everywhere, as samplers are
opaque types; at present, this code compiles without error in GLSL and
Metal. This is an issue for followup CLs.
GLSL's actual support for arrays of samplers is interesting and probably
too nuanced for us to try to emulate:
https://www.khronos.org/opengl/wiki/Data_Type_(GLSL)#Opaque_arrays
"Under GLSL version 3.30, Sampler arrays (the only opaque type 3.30
provides) can be declared, but they can only be accessed by compile-time
integral Constant Expressions. So you cannot loop over an array of
samplers, no matter what the array initializer, offset and comparison
expressions are.
Under GLSL 4.00 and above, array indices leading to an opaque value can
be accessed by non-compile-time constants, but these index values must
be dynamically uniform. The value of those indices must be the same
value, in the same execution order, regardless of any non-uniform
parameter values, for all shader invocations in the invocation group."
Change-Id: Ib382f5c3b563f996b3c8f1eb6b021b6d31fa9ce7
Bug: oss-fuzz:28107
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339159
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, GLSL and Metal code generators would emit a struct wherever
the type was first used in the code, regardless of where it was
originally defined or what scope the type needs to live in. This CL adds
a ProgramElement for struct definitions, so that structs will now appear
at the top-level as they were originally defined. In the case of Metal,
some special handling is also needed to handle the Globals struct
properly.
Not yet fully supported:
- No special handling for structs declared inside functions yet
- No support for structs in separate scopes with overlapping names
The severity of the remaining issues depends mostly on whether we want
to support structs inside functions in Runtime Effects.
Change-Id: Ia95d4529506cb3fa6da63f5cb548199a93e1c0c5
Bug: skia:10922, skia:10923, skia:10925, skia:10926
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338600
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Enums are an SkSL-only concept--when we output code, we emit plain
IntLiterals--so the fix is simply to ignore the Enum program element
when we encounter it. This is what GLSLCodeGen does as well.
Also added a unit test to confirm that enums work normally, and that
enums are subject to optimization and static-comparison checks just as
ints would be.
Change-Id: Ic4f8da7a27983add9eb41b936d46f6638d22bd4b
Bug: skia:11003
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338800
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
^^ is not an operator in Metal. != can be used for the same purpose.
Change-Id: If75b000076ebe0aa81d0ab354a8ae33e6ed52101
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339156
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>
Conceptually these should be no-ops, but hopefully could improve
performance slightly when compiling very simple programs.
Change-Id: I87f560fa6af817e7cf39fa920d04fe62d40ca79b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338599
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
From the perspective of a SPIR-V opcode stream, ^^ is equivalent to !=,
so TK_LOGICALXOR can share the existing logic with TK_NEQ. (There are
differences in precedence and in supported types, but those were shaken
out at the IR-gen/compilation stages.)
Change-Id: I541a5ecfa603a07b256132fd1522f91941de6b20
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338351
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, we allowed unary minus on numbers and vectors (of any type).
Now, we allow them on numbers and vectors of numbers.
Also updated the Boolean arithmetic error test to cover scalars as well
as vectors.
Change-Id: Ie74d1f3bfc1e9353e04c6f8e468fa20e0cbba16f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338396
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
GLSL does not allow most binary operations on bvec types; we can now
detect these and properly flag them as errors.
Note that `determine_binary_type` was also refactored. It originally
started with an enormous omni-switch over every possible Token type,
used to set various bools describing the type of binary expression at
hand. Instead of one big switch, this has been refactored into several
small switches in standalone functions that simply switch on the op and
immediately return true or false. Conceptually this seems like more
work (checking the op multiple times), but these tiny switches actually
boil down to little branchless shift-and-mask functions, so in practice
they should be quite efficient compared to the original omni-switch.
Change-Id: I81b473d98c65da1edd136f35fc8f656261f8930d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338346
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
These checks are made very frequently; it significantly eases
readability to have dedicated accessor methods, versus the verbose
`x.typeKind() == Type::TypeKind::kFoobar`.
Change-Id: I812b95f871cee436ccd3a5982c404f83563d44e5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338317
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>
Auto-Submit: John Stiles <johnstiles@google.com>
This is very unlikely to occur in real-world code, as it's somewhat
nonsense to use the comma operator in this way. However, it's better to
fail cleanly than to assert.
Change-Id: I76481cd8a993cb1a798ee16956400a512efd4c15
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/337636
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Fiddled with the logic a bit so that when we're in unit test mode, the
output still includes all of the SPIR-V (as well as the validation error
message), so that tracking them down is easier.
Bug: skia:10694
Change-Id: I15e7777af3d268a5952765dbe5d63612cad0ac07
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338320
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This is a reland of 0d5d956f7b
Original change's description:
> SkSL: Test/implement "geometric" intrinsics
>
> Bug: skia:10913
> Change-Id: Ie82354b05db141c8ab90b1a615ddfada4f71a98b
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335049
> Commit-Queue: Brian Osman <brianosman@google.com>
> Reviewed-by: John Stiles <johnstiles@google.com>
Bug: skia:10913
Change-Id: I103dd2efbbab0efeac2be786d7e8f913d5c4b22a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338158
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Fix code generation for Metal and Vulkan with geometric
intrinsics that have scalar versions in GLSL/SkSL, but no
native support in MSL/SPIR-V.
Change-Id: Id4538a00172e0d233ad9d5ed8d33db6436b83208
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338276
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Previously, we assumed that if a vector in `is_constant` was not made of
floats, it must be made of integers. This ignores that boolean vectors
also exist. The original code would abort when `getIVecComponent` was
called on a bool vector.
There is another bug here--arithmetic operators on bool types should be
disallowed entirely. That will be addressed in later CLs.
Change-Id: I78781d839abde9376917fd92f2fe6311a1a58b02
Bug: oss-fuzz:27808
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338055
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
We have built-in methods for determining whether a type is an int,
float, signed, unsigned, matrix, vector, etc. For some reason, however,
the lowly boolean never received similar treatment. Now, booleans are a
first-class citizen and can be identified by calling `isBoolean` instead
of doing a string compare or looking at the Context type pointers.
(I did do a quick search to make sure that kNonnumeric wasn't used
anywhere else to check for Boolean-ness.)
Change-Id: I35c0e3c7530c13e2c4e307a70272d298ce6b44bc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338042
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 e81fb87bb4.
Reason for revert: checking results with less-aggressive inliner
Original change's description:
> Revert "Simplify _blend_set_color_saturation, removing an instruction."
>
> This reverts commit ed289e777c.
>
> Reason for revert: causing strange artifacts, only on Adreno
>
> Original change's description:
> > Simplify _blend_set_color_saturation, removing an instruction.
> >
> > This tightens up our intrinsics slightly; after inlining, it eliminates
> > one scratch variable. (We no longer need to copy `sda` into `hueColor`
> > as hueColor is now unchanged.)
> >
> > Change-Id: Iece5ba2fe11cde54481704a1787114a2c2a66d9b
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/336599
> > Commit-Queue: John Stiles <johnstiles@google.com>
> > Commit-Queue: Brian Osman <brianosman@google.com>
> > Auto-Submit: John Stiles <johnstiles@google.com>
> > Reviewed-by: Brian Osman <brianosman@google.com>
>
> TBR=brianosman@google.com,johnstiles@google.com
>
> Change-Id: Ica506467b0a4e03d0cbe482034acfa2d9f8d2c16
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/337560
> Reviewed-by: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,johnstiles@google.com
Change-Id: Ia93263f3269c057e7eaa69ca2b05e783d18c0199
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/337944
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Previously, we'd gauge suitability for inlining by counting the nodes in
a function; past a certain limit, the function was considered "too big."
Now, we also incorporate the number of times that function is called.
So if a function is called three times, and its size is 20 nodes, it
would be considered to have an inlining cost of 60 (3 * 20) instead of
20.
This should tamp down the aggressive nature of the inliner in cases like
gaussian convolution or complicated blends, and will hopefully satisfy
Pinpoint.
No change visible in Nanobench (which doesn't test any of these sorts of
patterns, but certainly inlines things): http://screen/AwD5hkgkEfjVx4g
Change-Id: Ie5e32898245ac854adb9ddd52d87001df6a67125
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/337676
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
- float4(float2(1, 2), 3, 4) --> float4(1, 2, 3, 4)
- half3(z, half2(fn(x), y*2)) --> half3(z, fn(x), y*2)
Single-argument constructors will be ignored by this optimization; these
might be casts or splats.
This had an unexpected side benefit of simplifying some Metal output,
as we need to output fewer Metal matrix construction helper functions
when matrices use more simple scalars for construction.
Change-Id: I0a161db060c107e35247901619291bf83801cb11
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/337400
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This reverts commit ed289e777c.
Reason for revert: causing strange artifacts, only on Adreno
Original change's description:
> Simplify _blend_set_color_saturation, removing an instruction.
>
> This tightens up our intrinsics slightly; after inlining, it eliminates
> one scratch variable. (We no longer need to copy `sda` into `hueColor`
> as hueColor is now unchanged.)
>
> Change-Id: Iece5ba2fe11cde54481704a1787114a2c2a66d9b
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/336599
> Commit-Queue: John Stiles <johnstiles@google.com>
> Commit-Queue: Brian Osman <brianosman@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
TBR=brianosman@google.com,johnstiles@google.com
Change-Id: Ica506467b0a4e03d0cbe482034acfa2d9f8d2c16
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/337560
Reviewed-by: John Stiles <johnstiles@google.com>
These are not actually supported operators in GLSL, Metal or SPIR-V and
we don't emulate them. Their absence was causing SPIR-V to fail the
Operators.sksl test.
Change-Id: Ia6933788392aea48836b7be19e32b9969805f254
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/337185
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, the code which calculated Constructor constant values
assumed that a constant-value PrefixExpression would always have an
operand of Constructor. It turns out that another valid case is multiple
PrefixExpressions nested within each other (representing repeated
negation). Updated the code to work regardless of the type of the prefix
operand.
Change-Id: Ic9bf54725ae59330ac817bc4ec7a64def384ab54
Bug: oss-fuzz:27663
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/337177
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
We now have SPIR-V golden outputs for `blend` and `shared` tests.
This exposes a handful of SPIR-V limitations for us to address.
Change-Id: Ie5278889b8a61432403d06231b17765885bee0ac
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/337182
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: I4eb9fb50e645f503d71cd6423bfc38076a7c2119
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/336777
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:10913
Change-Id: Ie82354b05db141c8ab90b1a615ddfada4f71a98b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335049
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
We were always pre-loading the fragment and vertex modules, but deferred
loading all others. Those two take up about 300 KB of heap. Now, all
modules are deferred, so compiler instances that don't need them (like
the one used for runtime effects) are much smaller.
Now that we can get better fine-grained numbers, added two more
benchmarks, to track actual baseline usage, plus the usage in the two
most likely configurations.
Change-Id: Idfbcd52c8afee566ac42ab827c80c940f91c4ad7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/337176
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This allows swizzle removal to apply in more cases; in particular, we
can now optimize away extra swizzles caused by zero/one swizzle-
components quite effectively.
The "trivial expression" code was lifted from the inliner. Some subtle
changes in trivial-expression determination affect the inliner's results
in boring, non-meaningful ways. In particular, multi-argument
constructors containing all-constant values are now considered trivial,
whereas previously only single-argument constructors made the trivial-
ness cut. This allows the inliner to propagate some values that it
wouldn't have before.
Change-Id: I9a009b6803d9ac9595d65538252ba81c2b7166a7
Bug: skia:10954
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/336156
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
There is no longer anything IRNode-centric about SkSLPool; it is not
optimized around specific allocation sizes, and could be used to
allocate anything that is associated with a Program. Update comments and
function names to reflect this.
Change-Id: Ica1a78f7415edb25b93a93d00fe54557883c5eed
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334676
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, any builtin functions would be optimized as a side-effect of
optimizing programs that used them. Now that shared elements aren't
being optimized in that way, we explicitly optimize any shared modules
when they are first created. We don't remove dead elements, but we
we do substitute settings, simplify, and inline.
Bug: skia:10905
Change-Id: I701b5e9f52fb880ef3e6f4c67694d08602f47e95
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/336440
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
These cleanups were reverted, as they were part of the CL that added
`--` delimiters to skslc. This CL reinstates the cleanups, but does not
reinstate `--` delimited multiple-command-line support in skslc.
Change-Id: Id70ed87aa239b46d232492fc48791158b35512f3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/336677
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
These assertions were all currently reachable simply by compiling the
test code in /tests/sksl/shared/. Presumably many of these will actually
require a better fix going forward.
Change-Id: If59e0bfa1b248a5db9a79def736d437a9e5f7ee4
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/336676
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
The fix submitted at http://review.skia.org/335868 did not support
casts. The fuzzer discovered this shortcoming right away.
Change-Id: I2f5166528cee41367348564d4e664476fd5704ff
Bug: oss-fuzz:27650
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/336656
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This tightens up our intrinsics slightly; after inlining, it eliminates
one scratch variable. (We no longer need to copy `sda` into `hueColor`
as hueColor is now unchanged.)
Change-Id: Iece5ba2fe11cde54481704a1787114a2c2a66d9b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/336599
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>
Instead, add them to the shared program element list of any program that
needs them.
Bug: skia:10905
Change-Id: Ieb470af65eb254154d238554eecffdcbbf268cf1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335867
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Change program iteration so that default iteration (over owned & shared
elements) only permits const access. Add a separate non-const iterator
that only visits owned elements.
Initially, nothing is being placed in the shared list. Follow-up CLs
will move builtin variable declarations, builtin functions, etc.
Bug: skia:10905
Change-Id: I9a5b11170117bad3ff6a43aab780c1189904417c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330477
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
When values from the same argument are used consecutively by the outer
swizzle, they can be merged in the inner swizzle. Merging isn't always
possible, of course, but it will be used where it can be:
`half4(1, colRGB).yzwx` --> `half4(colRGB.xyz, 1)`
`half4(1, colRGB).yxzw` --> `half4(colRGB.x, 1, colRGB.yz)`
Change-Id: Id164b046bc15022ded331c06d722f1ae3605a3bd
Bug: skia:10954
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335872
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>
This will reorder constructors with swizzles applied, such as
`half4(1, 2, 3, 4).xxyz` --> `half4(1, 1, 2, 3)`
`half4(1, colRGB).yzwx` --> `half4(colRGB.x, colRGB.y, colRGB.z, 1)`
Note that, depending on the swizzle components, some elements of the
constructor may be duplicated and others may be eliminated. The
optimizer makes sure to leave the swizzle alone if it would duplicate
anything non-trivial, or if it would eliminate anything with a side
effect.
Change-Id: I470fda217ae8cf5828406b89a5696ca6aebf608d
Bug: skia:10954
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335860
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This was found at https://oss-fuzz.com/testcase-detail/5155684475469824
but the associated oss-fuzz issue ID appears to be misdirected (it's
showing oss-fuzz:24498, an unrelated issue).
PrefixExpressions can return true for `isCompileTimeConstant` but did
not implement `compareConstant`; the fuzzer discovered this. Because
compile-time constants can only be compared if they are of the same
kind, this means that `compareConstant` is actually comparing a pair of
expressions that are both negated. These negations will just cancel
out, so `compareConstant` on a pair of PrefixExpressions can just call
`compareConstant` on the inner operand of each expression.
Change-Id: I7793e25314e6c8a74278b73299d310794baf71f4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335870
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 ByteCodeGenerator is needed for SkSL-via-skvm, but almost no one
needs the ByteCode interpreter.
Bug: b/172773885
Change-Id: Ia7b6768dbc00c6c78b971ba50f0b702536bbd5b1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/336016
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
The fuzzer managed to create a test case which temporarily evaluates to
expression `half2(half(0.2)) + 2` as it is optimized. This requires a
bunch of temporary nonsense math as the IR Generator is attempting to
simplify as it goes; various attempts to remove terms from the fuzzer
test-case would cause it to stop reproducing the error.
Constructor::getVecComponent assumed that any constructor with a single
scalar argument would always implement `getConstantFloat` and
`getConstantInt`; however, constructors themselves did not actually
implement these methods. This meant that nesting a scalar constructor
inside a non-scalar constructor would abort when it tried to deduce the
value inside the inner constructor.
This has been fixed by implementing `getConstantFloat` and
`getConstantInt` for Constructors. These methods will assert if the
constructor has more than one argument or is a non-scalar type. This
should allow any number of nested constructors, e.g.
`half4(half(half(half(1))))` should recursively evaluate properly,
should we somehow generate this as an intermediate expression.
Change-Id: Iaee4284cba03974443cd7b5dccfd7909c1a5f3a6
Bug: oss-fuzz:27614
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335868
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 3e1b771ce4.
Reason for revert: Not working on Windows.
Original change's description:
> Replace skslc worklist files with -- delimited command lines.
>
> Command lines with delimiters are a simpler approach; they don't require
> a scratch file to be created and parsed. (I didn't consider this
> approach until after implementing worklists.)
>
> This also fixes a minor issue with result codes when processing multiple
> files at once; in particular, unit tests can ignore compile errors, but
> regular fragment processor compilation should treat compile errors as
> fatal and stop the build.
>
> Change-Id: I3f153e7670d757c6b021bf60a260a2cd3f2090aa
> Bug: skia:10919
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334428
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
# Not skipping CQ checks because original CL landed > 1 day ago.
Bug: skia:10919
Change-Id: I0e4bae8a8e09c61eac4e79453fd38e5e81b29e89
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335858
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
The optimizer can now turn the expression `half4(1).xyz` into
`half3(1)`, or `half4(1).w` into `1`. This is actually a somewhat common
case when inlining chains of fragment processors, as inputs are often
overridden to `half4(1)` or `half4(0)`. This optimization also applies
to more complex cases, e.g.:
`half2(anyFunc(sqrt(2))).yxyx` --> `half4(anyFunc(sqrt(2)))`
Since the interior of the constructor is always evaluated once in either
case, it does not actually matter what the constructor contains.
Change-Id: I8d5f358502eaa8e35d4968e74fbd6b0ce2ab6365
Bug: skia:10954
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335818
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: Ibe3c3d27a112df8838bc86d6c2482277fdae62af
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335821
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Fixes a bug with ternary expressions in the pre-includes.
Change-Id: Ib277ce7d9f6a50ab3ee02610746af2672208afde
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335820
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This was slightly dangerous, and only used for tests. Fix the tests to
just put a Context on the stack.
Change-Id: Ifc3d600c3498e1dedeee8350f3284163f3195bec
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335819
Auto-Submit: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This was slightly complicated by the fact that this syntax indicates an
array with a known size:
float[] x = float[](1, 2, 3, 4);
Of course, the size is 4; it's just never explicitly stated in the
code. (The SkSL parser never actually deduces the size, but it doesn't
apparently have a need to; we don't do much in the way of optimization
for arrays.) However, this prevents us from simply failing whenever we
parse "[]" in non-builtin code; we need to keep scanning and see if the
variable is initialized. We already check this in the
ArrayConstructors.sksl test file.
Change-Id: I5b86958e81bd9bf5edf28a617cecf95c1875583e
Bug: skia:10957
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335240
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, the Type's fOffset was set to -1 during parsing, so any
errors related to the Type would be reported on line 1.
Change-Id: I9834f733bc763c5946b3ff81d8aef4807cdc13d1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335584
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 a followup to http://review.skia.org/335196. This detects opaque
types (samplers and textures) at parsing or IR generation time and
reports an error regardless of backend. This check occurs before Metal
or SPIR-V would have a chance to detect the error, so it changes their
output to a slightly more focused error message. The Metal/SPIR-V fix in
the prior CL is still a nice broad catch-all for preventing spurious
ABORTs, though.
Change-Id: I4cce92a8767d72b5d3d7277a8afde8ce5ce86db2
Bug: skia:10956
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335217
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>
Perfect-forwarding can be used to implement CREATE_NODE as a templated
function instead of a macro. All the other macros were only necessary
because they were somehow entangled with CREATE_NODE.
(In the case of CREATE_EMPTY_CHILD I don't believe there was any
rationale for using a macro at all, other than maintaining parity with
CREATE_CHILD.)
Change-Id: Ic011e94d9712430cd4b82772dc449bc07e7f6bde
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335278
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, MemoryLayout would ABORT if it encountered any types that
we can't layout in memory (e.g. opaque types like samplers). Instead of
an abort, this case is now detected cleanly and an error is reported
identifying the offending type.
This should unwedge the fuzzer, which appears to be very
enthusiatically generating interface blocks with nonsense types inside.
(Note that code generators which don't actually try to compute a memory
layout--that is, GLSL--will still accept these types. This should still
be caught and reported as an error, since it's still illegal in GLSL,
but that's for a future CL.)
Change-Id: I88a9649bcd8c75dadc8cca679f3c5e94570742bc
Bug: skia:10956, oss-fuzz:27525
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335196
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:10913
Change-Id: I845bfad9ef57b57f09bdf96e74dcdab86300b34d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334877
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
This prevents OOMing when given a pathological input, but is large
enough that almost all inputs should continue to compile as-is.
Change-Id: If5c46711b886ee08495bfd09af537e9dc7ea5649
Bug: skia:10945, oss-fuzz:27442
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334838
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Both of these have variants with mixed scalar/vector parameters. Ensure
that all parameters are vectorized, or we fail SPIR-V validation.
Bug: skia:10913
Change-Id: I1a5be7fc02695e4c7047b5b9c3c08d12b2071e21
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334896
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This fixes a fuzzer crash in Metal.
Private types aren't meant to be used directly; we can't generate a
valid MemoryLayout for them. We will now detect them during IR
generation and report an error. (Note that unreferenced structs
currently don't have any IR representation at all, so structs have to be
used somewhere in the code to trigger the error.)
Bug: oss-fuzz:27288
Change-Id: I432f0a69fbb54cd33ff5b90a9f3d4757a9370117
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334830
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Without an early return, the inliner tries to inline a function inside
of itself, eating up gigabytes of memory before hitting its inline
threshold.
This normally wouldn't be possible because functions are meant to be
fully assembled before they're added to the list of ProgramElements, so
the inliner shouldn't find a function as a candidate to be inlined into
itself at all. However, the fuzzer found that an existing function
could be extended by re-declaring it; in this case, it is findable as
a ProgramElement and becomes inlinable.
Change-Id: I4c02a7b52e4b75151b75c94cb70dfadb8e4c9e6b
Bug: oss-fuzz:27442
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334556
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Command lines with delimiters are a simpler approach; they don't require
a scratch file to be created and parsed. (I didn't consider this
approach until after implementing worklists.)
This also fixes a minor issue with result codes when processing multiple
files at once; in particular, unit tests can ignore compile errors, but
regular fragment processor compilation should treat compile errors as
fatal and stop the build.
Change-Id: I3f153e7670d757c6b021bf60a260a2cd3f2090aa
Bug: skia:10919
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334428
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Rather than adding unique Modifiers to a vector and then returning
vector indices as opaque Handle objects, we can instead add them to an
unordered_set and return back the address of the object inside the set.
This removes a lot of complexity and saves an indirection.
STL unordered_sets guarantee pointer stability, so this is safe:
https://en.cppreference.com/w/cpp/container/unordered_set/insert
"If rehashing occurs due to the insertion, all iterators are
invalidated. Otherwise iterators are not affected. References are not
invalidated."
Change-Id: I580cad12b3711d490baab417affb8895f7fa18e7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334598
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Bug: skia:10913
Change-Id: Id9f2b14ca7443b3375036d588849bc9a20d76e40
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334156
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This completes all of the "Angle and Trigonometry Functions"
Made the graphs larger in the GM, and laid them out to make better use
of space. Also changed how the shaders are assembled to support
two-argument functions (at least partially).
Bug: skia:10913
Change-Id: I3ae6288f76fb9b3200c843595b065a6afbf92230
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334416
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
skslc can now take a `.worklist` file as an input, containing multiple
"command lines" to run in sequence. compile_sksl_tests.py now assembles
a worklist file and runs skslc one time, rather than running skslc
once per each target. This improves compile times on Windows
significantly (where spawning skslc hundreds of times is much more
expensive than on Linux/Mac).
One subtle behavioral difference with .worklist files: if an error is
encountered, it is written to the output file instead of to stdout.
Previously, compile_sksl_tests was in charge of for capturing stdout
and overwriting the compiler output with the error message, but this
doesn't work when many files are being compiled (which errors are
associated with which files?)
This refactor exposed a minor latent bug--when encountering an error,
skslc would previously exit() immediately without closing its
FileOutputStream. This led to an assertion when exit() was replaced with
normal returns. Since FileOutputStream is only used by skslc, and in
every case the desired behavior is just to close the stream cleanly,
FileOutputStream now closes the file in its destructor instead of
asserting that we haven't done so.
Change-Id: Ia55baff0c11fe466923bde2e0c944df9f2ccd092
Bug: skia:10919
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334099
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This doesn't include the two-argument version of atan, but covers all
other intrinsics from section 8.1 of the GLES Shading Language 1.00
spec.
Several needed additional plumbing for the CPU backend, but all now
appear correct across CPU and GPU.
Bug: skia:10913
Change-Id: I9933ad549b9914d94c9973c702a06bb177be31b1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334103
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
This was dead code: there's no way to refer to this builtin
from runtime effects.
Change-Id: Ie339a2de064ab65a2b4e312a098cd7d3e42b1499
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334042
Auto-Submit: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Change-Id: I24db08698330df4c725accd7f15ea2f6b39c9818
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/333877
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This addresses a sanitizer issue discovered in
https://oss-fuzz.com/testcase-detail/4908118777266176 (it has not been
assigned an oss-fuzz bug number yet; coming soon)
This puts an upper bound on struct nesting, again to prevent memory-
layout and other recursive type-handling code from overflowing the
stack. Coincidentally, while researching GLSL behavior around this bug,
I learned that WebGL has a similar limitation but caps nested structs to
4 deep. (I could not find any documented GLSL upper bound.)
Note that both the GLSL and Metal outputs for StructMaxDepth are badly
malformed. (Structs cannot be embedded within another struct in GLSL;
structs SA7 and below are never declared in GLSL; the array list for SA7
is backwards in GLSL; Metal is missing structs SA1 through SA8; Metal
puts the array list on the type instead of the variable name.)
These issues will be addressed in separate CLs.
Change-Id: I0f1059b6faa400cd0647dd7010ec839f73779a36
Bug: skia:10922, skia:10923, skia:10925, skia:10926
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/333316
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This addresses a sanitizer issue discovered in
https://oss-fuzz.com/testcase-detail/4908118777266176 (it has not been
assigned an oss-fuzz bug number yet; coming soon)
We need to set some sort of limit here to avoid stack overflow. Eight
array dimensions seems like more than enough for any sort of code that
we might realistically need, but the limit is definitely flexible if we
wanted to increase it. (The fuzzer needed to generate a several-
hundred-dimensional array before encountering a crash.)
Change-Id: I3630ab40e47cc58a2280ba200b485e1958371fdc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/333160
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This resolves the fuzzer error, as the program will fail compilation
before reaching the SPIR-V translation stage at all.
Change-Id: Ia73af497b1f57314a29878f2d2a29dc80186e630
Bug: oss-fuzz:27300
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/333130
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This reverts commit 4cb5c5e172, and fixes
the Chromium issue by declaring sign(x) in sksl_public.sksl.
Original description:
This makes numerous internal and GLSL types or intrinsics hidden from
public (runtime effect) SkSL. In particular:
- Only core numeric types are visible to all program types. GLSL types
involving images, textures, and sampling are restricted to internal use.
- sk_Caps is no longer visible to runtime effects.
- The set of intrinsics available to runtime effects is now a separate,
curated list in sksl_public.sksl. It exactly matches the GLSL ES 1.00
spec order.
- The blend intrinsics are no longer visible, which also fixes a bug.
These are nice, but we're not going to offer them yet - they involve
enums, which creates complications.
Bug: skia:10680
Bug: skia:10709
Bug: skia:10913
Cq-Include-Trybots: luci.chromium.try:linux-chromeos-rel,linux-rel
Change-Id: I42deeeccd725a9fe18314d091ce253404e3572e2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/332750
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
Auto-Submit: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
We already allowed narrowing type conversions, so GLSL type aliases
could be used almost exclusively. This fixes the last spot that a
client would be forced to use half4 rather than vec4.
Bug: skia:10679
Change-Id: Ie9cfc161650b238678861b9b126ce586c229162d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/332743
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This implements constant folding optimizations on int vectors
(== != + - * /) that were previously only supported on float vectors.
Bug: skia:10908
Change-Id: Ibf61ab43eb7ae2ce8e99cce21cc55777359817e5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/332424
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This gives template code a generic way to access IntLiteral/
getIVecComponent or FloatLiteral/getFVecComponent based on their
template type.
(Interestingly, we have BoolLiterals and support vectors of bools, but
don't have any paths in the optimizer that support constant-folding bool
vectors, so we don't have a getBVecComponent.)
Change-Id: I7af754f77544716cf6cd5c6703a07fb3dd1d5173
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/332742
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit bea0dc67f4.
No-Try: true
Change-Id: I7b000de199dc23bd3719a4ee835b2a8d3c93fefd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/332747
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This reverts commit 1277971939.
Change-Id: I7985ef22ddd19adcab468acc684b330ce6978c8d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/332738
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>
This makes numerous internal and GLSL types or intrinsics hidden from
public (runtime effect) SkSL. In particular:
- Only core numeric types are visible to all program types. GLSL types
involving images, textures, and sampling are restricted to internal use.
- sk_Caps is no longer visible to runtime effects.
- The set of intrinsics available to runtime effects is now a separate,
curated list in sksl_public.sksl. It exactly matches the GLSL ES 1.00
spec order.
- The blend intrinsics are no longer visible, which also fixes a bug.
These are nice, but we're not going to offer them yet - they involve
enums, which creates complications.
Bug: skia:10680
Bug: skia:10709
Bug: skia:10913
Change-Id: I8fa1c94f6e4899f38530bb9cff33d147f6983ab3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/332597
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This reverts commit a2d6b31f66.
Reason for revert: breaking bots
Original change's description:
> Additional SkSL benches
>
> This adds additional benchmarks to give us better insight into how long
> the various compilation phases take. The four benches per size now
> cover just parsing, parsing + converting to IR, parsing + converting to
> IR + optimizing, and finally parsing + converting to IR + optimizing +
> generating GLSL.
>
> Change-Id: I7099a5a9f40ae5031e330dc4e1bb08c2a20ada63
> Bug: skia:10805
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/332262
> Reviewed-by: Brian Osman <brianosman@google.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: Idb3e3082d11f72978131068b2229ce2578d645c3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:10805
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/332601
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
This adds additional benchmarks to give us better insight into how long
the various compilation phases take. The four benches per size now
cover just parsing, parsing + converting to IR, parsing + converting to
IR + optimizing, and finally parsing + converting to IR + optimizing +
generating GLSL.
Change-Id: I7099a5a9f40ae5031e330dc4e1bb08c2a20ada63
Bug: skia:10805
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/332262
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
This CL improves on the previous fix for oss-fuzz:26789 by actually
propagating the negation from the PrefixExpression inside the
constructor, which unblocks further optimizations.
Interestingly, this fix also exposes a further missing optimization--we
optimize away comparisons of constant-vectors for floats, but fail to
do the same for ints.
Change-Id: I9d4cb92b10452a74db96ff264322cdc8a8f2a41f
Bug: oss-fuzz:26830, oss-fuzz:26789, skia:10908
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/332263
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
The previous behavior leaked Skia-internal concepts into public SkSL.
Users coming from GLSL will expect that bindable/sampleable objects are
uniform (just like texture2D). This keeps the old support around (and
tested), but updates all of our examples to use 'uniform'.
Bug: skia:10679
Change-Id: I0c98162f5e21dad7014d9778ceb26143d2f6030e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/332376
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This CL solves the fuzzer crash. Constant propagation of the negative
sign into the vector will be investigated in a followup CL.
This CL also adds a few cleanups into IRGenerator::constantFold.
Change-Id: If73a4fe2a5777265e7d43cc4f482653a38cb59af
Bug: oss-fuzz:26830, oss-fuzz:26789
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/332261
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This ties the caps to the compiler instance, paving the way for
pre-optimizing the shared code. Most of the time, the compiler is
created and owned the GPU instance, so this is fine. For runtime
effects, we now use the shared (device-agnostic) compiler instance
for the first compile, even on GPU. It's configured with caps that
apply no workarounds. We pass the user's SkSL to the backend as
cleanly as possible, and then apply any workarounds once it's part
of the full program.
Bug: skia:10905
Bug: skia:10868
Change-Id: Ifcf8d7ebda5d43ad8e180f06700a261811da83de
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/331493
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Previously, temp variables created by sample() calls were named after
the offset of the sample() call within the code. This was
straightforward but would fail if the sample() call were duplicated via
inlining of helper functions.
FP sample() temp variables are now named using a counter, starting from
zero and counting upwards.
Change-Id: I16f9a3426117677c0df13d15772320def99cc0d6
Bug: skia:10858
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/331415
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, when a prototype was parsed, this added a function
declaration to the symbol table, but the prototype itself was not
re-emitted during code generation. This meant that the final code might
not be valid, since the absence of prototypes meant that the code might
attempt to invoke a function before its declaration. Now, prototypes are
stored in the ProgramElement list and re-emitted during code generation
for GLSL/Metal/CPP. (SPIR-V doesn't name its functions at all.)
Change-Id: I76446c796000eb0b56f964d82457122182c28b87
Bug: skia:10872
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/331136
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
When eliminating a CFG node, we now flag its exit nodes; if our
optimization pass reaches one of those flagged nodes, we stop the
current optimization process in its tracks and initiate a rescan.
We do NOT recursively mark the exits of the exit nodes, so this fix is
reliant on the CFG being ordered in a non-chaotic fashion, but in
practice this seems to be sufficient for the CFGs we generate today.
Change-Id: I892805361c5f4297e02146f37a759dfda83f5488
Bug: oss-fuzz:26942
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/331597
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: Ief57d9c102b3c7658738920cdf54ccd4d21c5c5e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/331656
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>
This has two advantages:
- sksl_gpu was on the verge of overflowing 255 program elements anyway
- this makes it easier to skip ProgramElements when generating the
dehydrated data; in particular, we don't have any need to dehydrate
function prototypes, but if we just skip them, the count would be
wrong
Change-Id: Idbcdec53518e9e6f42473a73a53dae408ce7c980
Bug: skia:10872
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/331282
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>
This error was caused by an unbalanced symbol table push. This could
occur when an interface block encountered an error while parsing its
var-decls.
Change-Id: I910a980ac92fac7c0786c48b8dc3003ee3e75e5b
Bug: oss-fuzz:26700
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330896
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Profiling sksl_large showed a non-trivial amount of time was spent on
allocating vector<int>s related to swizzle components. This CL
essentially eliminates that cost, for a ~2% savings.
Nanobench before: http://screen/35m2rfy5B8h9eyg
Nanobench after: http://screen/4GuW6ipodyBL34h
Change-Id: I653b69bf1bfbcfdf048987edd23e6f14a5ef3fbc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330336
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This is a reland of f71e0be970
Original change's description:
> Moved SkSL data back into node classes
>
> The original goal of this rearchitecture had been to move all of the
> data into IRNode so that we could manage IRNode objects directly rather
> than std::unique_ptr<IRNode>. Other changes have rendered that original
> goal obsolete, so this is undoing most of the work that was done during
> this rearchitecture.
>
> Change-Id: Ic56ffb17bb013c8b4884d710215f5345a481468a
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330297
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
> Reviewed-by: John Stiles <johnstiles@google.com>
Change-Id: Ifec4777a42ef0f95f6edc418dcd46fd38c856fa5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330739
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 f71e0be970.
Reason for revert: breaking Build-Debian10-EMCC-wasm-Release-WasmGMTests
Original change's description:
> Moved SkSL data back into node classes
>
> The original goal of this rearchitecture had been to move all of the
> data into IRNode so that we could manage IRNode objects directly rather
> than std::unique_ptr<IRNode>. Other changes have rendered that original
> goal obsolete, so this is undoing most of the work that was done during
> this rearchitecture.
>
> Change-Id: Ic56ffb17bb013c8b4884d710215f5345a481468a
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330297
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
> Reviewed-by: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I7a043c8e3e5c711164303cf160846d7cf20ddfbe
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330736
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Change-Id: Idfbe128978575ab84b54485bffe2d82570ee099f
Bug: skia:10870
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330620
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
The original goal of this rearchitecture had been to move all of the
data into IRNode so that we could manage IRNode objects directly rather
than std::unique_ptr<IRNode>. Other changes have rendered that original
goal obsolete, so this is undoing most of the work that was done during
this rearchitecture.
Change-Id: Ic56ffb17bb013c8b4884d710215f5345a481468a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330297
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This is an experiment to see if Windows performance numbers are impacted
by removing the recycler entirely. When pools were > 300K, the cost of
continually creating and destroying the pool in a tight loop was
prohibitive, but now that they are 64K, it's possible that the pool
allocation is less expensive and recycling them might not be worth the
downsides.
Change-Id: I7cf4f00e539310da2f852f36419f12d6904727cf
Bug: skia:10865
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330436
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
GrBlockAllocators will hang onto an extra scratch node unless you ask
them to reset. When a Pool is detached from its thread, we know that
we're done with whatever work we needed to do, so it's a good time to
reset any scratch space and reclaim that memory.
Change-Id: I0ff2c4fad9f51a2f3dc911730abad77c06af8d57
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330276
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This is a reland of 67e1cf4b1d
The iOS 8 code path now compiles normally
Original change's description:
> Replace pooling mechanism with GrMemoryPool.
>
> This change is a wash for tests that could fit inside the previous
> hard-coded pool (512 nodes) and appears to be a 5% improvement for
> sksl_large. Larger programs would hypothetically show an even more
> significant improvement.
>
> When SK_SUPPORT_GPU is disabled, we disable pooling entirely and fall
> back to the system allocator. This is necessary because SkSL can exist
> without Ganesh (such as in the wasm+CanvasKit build).
>
> Nanobench: http://screen/4xJEzdGducRxGeq
>
> Change-Id: I71dc702a84ab5c163673e35ec651003d7d45dacd
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330219
> Commit-Queue: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: Iced330084f1ed8997e19bbee585422cb89e1c6b0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330404
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 67e1cf4b1d.
Reason for revert: iOS 8
Original change's description:
> Replace pooling mechanism with GrMemoryPool.
>
> This change is a wash for tests that could fit inside the previous
> hard-coded pool (512 nodes) and appears to be a 5% improvement for
> sksl_large. Larger programs would hypothetically show an even more
> significant improvement.
>
> When SK_SUPPORT_GPU is disabled, we disable pooling entirely and fall
> back to the system allocator. This is necessary because SkSL can exist
> without Ganesh (such as in the wasm+CanvasKit build).
>
> Nanobench: http://screen/4xJEzdGducRxGeq
>
> Change-Id: I71dc702a84ab5c163673e35ec651003d7d45dacd
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330219
> 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: I26dbd7f2d5348dd717c39fd0780ee5d140292e9a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330416
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This change is a wash for tests that could fit inside the previous
hard-coded pool (512 nodes) and appears to be a 5% improvement for
sksl_large. Larger programs would hypothetically show an even more
significant improvement.
When SK_SUPPORT_GPU is disabled, we disable pooling entirely and fall
back to the system allocator. This is necessary because SkSL can exist
without Ganesh (such as in the wasm+CanvasKit build).
Nanobench: http://screen/4xJEzdGducRxGeq
Change-Id: I71dc702a84ab5c163673e35ec651003d7d45dacd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330219
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This provides the optimizer with hints so that it can avoid virtual
calls in scenarios where the type is known.
(The ExternalValue leaf class is excluded from this change; unlike
most IRNodes, it is meant to be subclassed.)
Change-Id: I68ff3e2336cb478bcc546c9fe3640719a01d8ea7
Bug: skia:10886
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330223
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Herb Derby <herb@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Herb Derby <herb@google.com>
This is trying to dynamically set the array bounds for sk_in, based on
the primitive type. However, the sizes() field of the interface block
isn't even used for that - it's done with similar logic in
writeInterfaceBlock. Also, this was using the wrong size for several
primitive types.
Bug: skia:7733
Change-Id: Ic0925b51d86dcaea718373bb16d633cc2cd3398f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330222
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
To make this possible, the logic for maintaining a pool of a given node
size has been split into a separate `Subpool` class. We now have two
Subpool objects in the PoolData class; they are always adjacent in
memory, and we can still identify a pooled node just as easily by
verifying that the pointer address is within the range of one of our
Subpools. (Identifying which subpool requires one extra check.)
This change will allow us to pool a handful of larger allocations, which
will benefit almost every program since a handful of variables and
functions are universally necessary, even in simple code.
For now, the chosen size of a large node is just a guess (2x the small
node size); this can be adjusted easily once we have a better idea of
the expected sizes for our larger IRNodes, but for now this successfully
catches a significant number of allocations in `sksl_medium` and
`sksl_large`.
Change-Id: I08fb9c55c02162d4b56e72ff4e0923f2cf258651
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330124
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This is a reland of 22ef2257c8
This reland fixes an issue with ExternalValue nodes on 32-bit builds;
these should never be pooled, because we can't control their lifetimes.
Original change's description:
> Update the SkSL pool interface to take an allocation size.
>
> Since our end goal no longer has all IRNodes ending up as the exact same
> size in memory, it makes sense for the allocator function to take in a
> desired size. This also opens the door to separate "small" and "large"
> pools, if we want to add pooling support for large but semi-common
> things like Variables.
>
> Change-Id: If3dbe31588adeedede327c5967c344a19507b6fa
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329961
> Reviewed-by: Brian Osman <brianosman@google.com>
> Commit-Queue: Brian Osman <brianosman@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: If701ae4a5e18b66d4138bc09c9c8dc1a60579c90
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330105
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit 22ef2257c8.
Reason for revert: mysterious tree breakage
Original change's description:
> Update the SkSL pool interface to take an allocation size.
>
> Since our end goal no longer has all IRNodes ending up as the exact same
> size in memory, it makes sense for the allocator function to take in a
> desired size. This also opens the door to separate "small" and "large"
> pools, if we want to add pooling support for large but semi-common
> things like Variables.
>
> Change-Id: If3dbe31588adeedede327c5967c344a19507b6fa
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329961
> Reviewed-by: Brian Osman <brianosman@google.com>
> Commit-Queue: Brian Osman <brianosman@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I346948aeb407dc6e6e84835f68d0353e4869ddf7
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329965
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Since our end goal no longer has all IRNodes ending up as the exact same
size in memory, it makes sense for the allocator function to take in a
desired size. This also opens the door to separate "small" and "large"
pools, if we want to add pooling support for large but semi-common
things like Variables.
Change-Id: If3dbe31588adeedede327c5967c344a19507b6fa
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329961
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
When the test expression has a side-effect, but both the true and else
blocks are empty, the optimizer moves the test out to a standalone
ExpressionStatement. Updating the usage in that situation involves
traversing an IfStatement with no test.
Bug: oss-fuzz:26666
Change-Id: I2fb4004f2401784402040345df49a7d42e4aab5e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329960
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
There's no functional change here; it's just using a slightly higher-
level abstraction to pass the same payload.
Change-Id: Ife7efa038db5d6dbde5decae2be79ad9db877aba
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329782
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
GLSL requires that functions are declared before first use. In most
cases, we avoid the requirement for explicit prototypes by this by
strategically ordering our functions within the emitted code, but an FP
file might reference its helper functions in any order or have helper
functions that cross-invoke each other, so prototypes should be emitted.
Change-Id: I3b9e9c9ec4bd5be90b4f71f8165af45364facf30
Bug: skia:10872
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329781
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This is necessary to support function calls in FP files properly; in
some cases, functions can be referenced before they have been emitted,
and we need to be able to name them.
This CL resolves the remaining errors in GrRecursion.cpp. There are
still additional errors in GrNestedCall.cpp that will be fixed in
followup CLs.
Change-Id: Iec98ef02ea6a98a9945a4e0e3cfa3537dff01305
Bug: skia:10684, skia:10872
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329676
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This backs off a bit on the "stuff everything into IRNode" push, moving
the data of the big nodes back into them and significantly cutting the
size of IRNode. This isn't a complete revert of the changes to these
nodes - now that all of the fields have been hidden behind accessor
methods, it's merely an implementation detail where the data actually
lives, so these changes are much smaller and more targeted.
Change-Id: I84c770245f26dfe36651d0f482543505bd7f71ef
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329629
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Previously, we'd just emit functions with `%s` in their function bodies.
Also, the fFormatArgs wouldn't get cleaned up, so subsequent code would
fill in the wrong format arguments in each place.
There are still problems with function calls (`sample` is broken;
function names are accessed before they've been declared or initialized)
but this is a step in the right direction.
Bug: skia:10684
Change-Id: I7399a71d44ebba5049703484ec9933dffbe8e2b9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329417
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Automatically computed once IR generation is complete, and then used
throughout optimization and inlining, and the backend generators.
Optimization and inlining are reworked to keep the data up-to-date as
they edit the IR.
Bug: skia:10589
Change-Id: Iaded42d8157732dd6fe05f74c5b7bb8366916635
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/328382
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This is large enough to hold sksl_medium in its entirety; sksl_large
overflows somewhat, but it's still much better than nothing.
Change-Id: Ief22a94cc7f554ecc289905e9ccc20594eab6819
Bug: chromium:1141863
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329456
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
In cases where multiple variables were declared on a single line, it is
legal for variable initialization-expressions to reference variables
declared earlier in the var-decl statement. It is NOT legal for the
inliner to move those references up to the previous statement, where the
variable doesn't exist yet.
This is mitigated by disabling the IRGenerator inliner for var-decls
past the first one in a var-decls statement. (The optimizer will still
pass over this code later and is able to inline it correctly, if it is
worth doing.)
Change-Id: I7a0d45eab20e30ed9f6b2f5c1251b6e0d8eeaea3
Bug: oss-fuzz:26167
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329357
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
When a Program is freed, rather than immediately disposing of its Pool,
it now sends it to Pool::Recycle, which holds onto it. If Pool::Create
is called, it satisfies the request by simply handing back the recycled
pool. Only one pool is kept in recycle storage at a time--recycling
more than one pool in a row will cause all but one to be freed. To avoid
holding onto Pool memory indefinitely, pool recycle storage is cleaned
up whenever a Compiler is destroyed.
Change-Id: I21c1ccde84507e344102d05506d869e62ca095a6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329175
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I1b14e4c8de0ca60a0ebbdbc225befc5074e8de22
Bug: skia:10862
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329177
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 simplifies the signatures of several methods and makes it easier to
ignore errors we have already reported.
Change-Id: I767ca22a66e69fe72557c2560ef84f5dbe339eb8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329220
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This reverts commit f73091bb81.
Reason for revert: rolling forward after fixing prior CL
Original change's description:
> Revert "Add pooling support on iOS."
>
> This reverts commit 38a93e622b.
>
> Reason for revert: need revert first pool change
>
> Original change's description:
> > Add pooling support on iOS.
> >
> > This replaces the `thread_local` attribute with `pthread_setspecific`
> > and `pthread_getspecific`. I don't have easy access to iOS 8/9 for
> > testing purposes, but on Mac OS X, this implementation works and
> > benchmarks the same as the `thread_local` implementation.
> >
> > Change-Id: I86db88c24d59d946adb66141b32733ebf5261c76
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/328837
> > Reviewed-by: Brian Osman <brianosman@google.com>
> > Commit-Queue: Brian Osman <brianosman@google.com>
> > Auto-Submit: John Stiles <johnstiles@google.com>
>
> TBR=brianosman@google.com,adlai@google.com,johnstiles@google.com
>
> Change-Id: Ic06f9e32e524b2be601ee21a5da605fd19aaa64b
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329164
> Reviewed-by: Greg Daniel <egdaniel@google.com>
> Commit-Queue: Greg Daniel <egdaniel@google.com>
TBR=egdaniel@google.com,brianosman@google.com,adlai@google.com,johnstiles@google.com
Change-Id: I0e021e9304ee88d6a29739c287eb515abff8b8a4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329173
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This is a reland of e16eca95f5
This fixes the no-op (iOS) implementation of CreatePoolOnThread.
Original change's description:
> Create a basic IRNode pooling system.
>
> Allocations are redirected by overriding `operator new` and `operator
> delete` on the IRNode class. This allows us to use our existing
> `unique_ptr` and `make_unique` calls as-is. The Pool class is simple;
> it holds a fixed number of nodes and recycles them as they are returned.
>
> A fixed pool size of 2000 nodes was chosen. That is large enough to hold
> the contents of `sksl_large` during compilation, but it can be
> overflowed by very large shaders, or if multiple programs are converted
> at the same time. Exhausting the pool is not a problem; if this happens,
> additional nodes will be allocated via the system allocator as usual.
> More elaborate schemes are possible but might not add a lot of value.
>
> Thread safety is accomplished by placing the pool in a `thread_local`
> static during a Program's creation and destruction; the pool is freed
> when the program is destroyed. One important consequence of this
> strategy is that a program must free every node that it allocated during
> its creation, or else the node will be leaked. In debug, leaking a node
> will be detected and causes a DEBUGFAIL. In release, the pool will be
> freed despite having a live node in it, and if that node is later freed,
> that pointer will be passed to the system `free` (which is likely to
> cause a crash).
>
> In this CL, iOS does not support pooling, since support for
> `thread_local` was only added on iOS 9. This is fixed in the followup
> CL, http://review.skia.org/328837, which uses pthread keys on iOS.
>
> Nanobench shows ~15% improvement:
> (last week) http://screen/5CNBhTaZApcDA8h
> (today) http://screen/8ti5Rymvf6LUs8i
>
> Change-Id: I559de73606ee1be54e5eae7f82129dc928a63e3c
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/326876
> Commit-Queue: John Stiles <johnstiles@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I8623a574a7e92332ff00b83982497863c8953929
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329171
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 5b09e6a007.
Reason for revert: breaking g3
Original change's description:
> Reland "Create a basic IRNode pooling system."
>
> This is a reland of e16eca95f5
>
> Original change's description:
> > Create a basic IRNode pooling system.
> >
> > Allocations are redirected by overriding `operator new` and `operator
> > delete` on the IRNode class. This allows us to use our existing
> > `unique_ptr` and `make_unique` calls as-is. The Pool class is simple;
> > it holds a fixed number of nodes and recycles them as they are returned.
> >
> > A fixed pool size of 2000 nodes was chosen. That is large enough to hold
> > the contents of `sksl_large` during compilation, but it can be
> > overflowed by very large shaders, or if multiple programs are converted
> > at the same time. Exhausting the pool is not a problem; if this happens,
> > additional nodes will be allocated via the system allocator as usual.
> > More elaborate schemes are possible but might not add a lot of value.
> >
> > Thread safety is accomplished by placing the pool in a `thread_local`
> > static during a Program's creation and destruction; the pool is freed
> > when the program is destroyed. One important consequence of this
> > strategy is that a program must free every node that it allocated during
> > its creation, or else the node will be leaked. In debug, leaking a node
> > will be detected and causes a DEBUGFAIL. In release, the pool will be
> > freed despite having a live node in it, and if that node is later freed,
> > that pointer will be passed to the system `free` (which is likely to
> > cause a crash).
> >
> > In this CL, iOS does not support pooling, since support for
> > `thread_local` was only added on iOS 9. This is fixed in the followup
> > CL, http://review.skia.org/328837, which uses pthread keys on iOS.
> >
> > Nanobench shows ~15% improvement:
> > (last week) http://screen/5CNBhTaZApcDA8h
> > (today) http://screen/8ti5Rymvf6LUs8i
> >
> > Change-Id: I559de73606ee1be54e5eae7f82129dc928a63e3c
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/326876
> > Commit-Queue: John Stiles <johnstiles@google.com>
> > Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
> > Auto-Submit: John Stiles <johnstiles@google.com>
>
> Change-Id: I114971e8e7ac0fabaf26216ae8813eeeaad0d4a2
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329086
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: Ie77a23366f2ba52fcbb0a751d11ca2792790a30c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329165
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
This reverts commit 38a93e622b.
Reason for revert: need revert first pool change
Original change's description:
> Add pooling support on iOS.
>
> This replaces the `thread_local` attribute with `pthread_setspecific`
> and `pthread_getspecific`. I don't have easy access to iOS 8/9 for
> testing purposes, but on Mac OS X, this implementation works and
> benchmarks the same as the `thread_local` implementation.
>
> Change-Id: I86db88c24d59d946adb66141b32733ebf5261c76
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/328837
> Reviewed-by: Brian Osman <brianosman@google.com>
> Commit-Queue: Brian Osman <brianosman@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,adlai@google.com,johnstiles@google.com
Change-Id: Ic06f9e32e524b2be601ee21a5da605fd19aaa64b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329164
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
This replaces the `thread_local` attribute with `pthread_setspecific`
and `pthread_getspecific`. I don't have easy access to iOS 8/9 for
testing purposes, but on Mac OS X, this implementation works and
benchmarks the same as the `thread_local` implementation.
Change-Id: I86db88c24d59d946adb66141b32733ebf5261c76
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/328837
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I433159717b831e9dbfc251c38687572bdc45c959
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329037
Auto-Submit: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This is a reland of e16eca95f5
Original change's description:
> Create a basic IRNode pooling system.
>
> Allocations are redirected by overriding `operator new` and `operator
> delete` on the IRNode class. This allows us to use our existing
> `unique_ptr` and `make_unique` calls as-is. The Pool class is simple;
> it holds a fixed number of nodes and recycles them as they are returned.
>
> A fixed pool size of 2000 nodes was chosen. That is large enough to hold
> the contents of `sksl_large` during compilation, but it can be
> overflowed by very large shaders, or if multiple programs are converted
> at the same time. Exhausting the pool is not a problem; if this happens,
> additional nodes will be allocated via the system allocator as usual.
> More elaborate schemes are possible but might not add a lot of value.
>
> Thread safety is accomplished by placing the pool in a `thread_local`
> static during a Program's creation and destruction; the pool is freed
> when the program is destroyed. One important consequence of this
> strategy is that a program must free every node that it allocated during
> its creation, or else the node will be leaked. In debug, leaking a node
> will be detected and causes a DEBUGFAIL. In release, the pool will be
> freed despite having a live node in it, and if that node is later freed,
> that pointer will be passed to the system `free` (which is likely to
> cause a crash).
>
> In this CL, iOS does not support pooling, since support for
> `thread_local` was only added on iOS 9. This is fixed in the followup
> CL, http://review.skia.org/328837, which uses pthread keys on iOS.
>
> Nanobench shows ~15% improvement:
> (last week) http://screen/5CNBhTaZApcDA8h
> (today) http://screen/8ti5Rymvf6LUs8i
>
> Change-Id: I559de73606ee1be54e5eae7f82129dc928a63e3c
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/326876
> Commit-Queue: John Stiles <johnstiles@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I114971e8e7ac0fabaf26216ae8813eeeaad0d4a2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329086
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This fixes a potential leak of the symbol table. (The leak would
eventually resolve itself once the IRGenerator were destroyed, but it
does outlast the compilation step.)
Change-Id: I6b2e303f00a3331fccbd8421a5173defd352f022
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/328985
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This reverts commit e16eca95f5.
Reason for revert: ASAN error on fuzzer
https://status.skia.org/logs/snBeMRUkDrwDYbnm2SAG/7ad38736-d579-4e94-bc10-87c002f3f7d6/fd7b6ea1-5d36-4612-85d1-88462a5271f7
Original change's description:
> Create a basic IRNode pooling system.
>
> Allocations are redirected by overriding `operator new` and `operator
> delete` on the IRNode class. This allows us to use our existing
> `unique_ptr` and `make_unique` calls as-is. The Pool class is simple;
> it holds a fixed number of nodes and recycles them as they are returned.
>
> A fixed pool size of 2000 nodes was chosen. That is large enough to hold
> the contents of `sksl_large` during compilation, but it can be
> overflowed by very large shaders, or if multiple programs are converted
> at the same time. Exhausting the pool is not a problem; if this happens,
> additional nodes will be allocated via the system allocator as usual.
> More elaborate schemes are possible but might not add a lot of value.
>
> Thread safety is accomplished by placing the pool in a `thread_local`
> static during a Program's creation and destruction; the pool is freed
> when the program is destroyed. One important consequence of this
> strategy is that a program must free every node that it allocated during
> its creation, or else the node will be leaked. In debug, leaking a node
> will be detected and causes a DEBUGFAIL. In release, the pool will be
> freed despite having a live node in it, and if that node is later freed,
> that pointer will be passed to the system `free` (which is likely to
> cause a crash).
>
> In this CL, iOS does not support pooling, since support for
> `thread_local` was only added on iOS 9. This is fixed in the followup
> CL, http://review.skia.org/328837, which uses pthread keys on iOS.
>
> Nanobench shows ~15% improvement:
> (last week) http://screen/5CNBhTaZApcDA8h
> (today) http://screen/8ti5Rymvf6LUs8i
>
> Change-Id: I559de73606ee1be54e5eae7f82129dc928a63e3c
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/326876
> Commit-Queue: John Stiles <johnstiles@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I625d95a14057727b297c0bfc5b98bcd78ad8572c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/328906
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Switch statements were not caught when originally implementing
http://review.skia.org/328384.
Change-Id: Iff21e5743bf6a604e13c45a736799b4929844472
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/328900
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>
Allocations are redirected by overriding `operator new` and `operator
delete` on the IRNode class. This allows us to use our existing
`unique_ptr` and `make_unique` calls as-is. The Pool class is simple;
it holds a fixed number of nodes and recycles them as they are returned.
A fixed pool size of 2000 nodes was chosen. That is large enough to hold
the contents of `sksl_large` during compilation, but it can be
overflowed by very large shaders, or if multiple programs are converted
at the same time. Exhausting the pool is not a problem; if this happens,
additional nodes will be allocated via the system allocator as usual.
More elaborate schemes are possible but might not add a lot of value.
Thread safety is accomplished by placing the pool in a `thread_local`
static during a Program's creation and destruction; the pool is freed
when the program is destroyed. One important consequence of this
strategy is that a program must free every node that it allocated during
its creation, or else the node will be leaked. In debug, leaking a node
will be detected and causes a DEBUGFAIL. In release, the pool will be
freed despite having a live node in it, and if that node is later freed,
that pointer will be passed to the system `free` (which is likely to
cause a crash).
In this CL, iOS does not support pooling, since support for
`thread_local` was only added on iOS 9. This is fixed in the followup
CL, http://review.skia.org/328837, which uses pthread keys on iOS.
Nanobench shows ~15% improvement:
(last week) http://screen/5CNBhTaZApcDA8h
(today) http://screen/8ti5Rymvf6LUs8i
Change-Id: I559de73606ee1be54e5eae7f82129dc928a63e3c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/326876
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
PLS and discard-only shaders are the only time this has an impact,
and it doesn't seem like a problem to have the declaration?
Removes one use of variable reference counts, which are going to be
refactored.
Change-Id: Idb8d06087eed56070252ee02dcf907bf0d24c5a1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/328796
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Not yet used as of this CL.
Change-Id: Ic82ab5e2e2ca17fb11c16e22cfa6b7ad5ff74c77
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/328657
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
This is conceptually very similar to http://review.skia.org/328384, but
the inliner doesn't use `clone()` when it clones a node.
Change-Id: I7456b63687ce2f93a7980fb101dfc97e143a378f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/328817
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>
After an IRNode is cloned, callers expect to be able to safely mutate
its SymbolTable, so it can't be left with a built-in one.
Change-Id: If658fd11ad580da552f9d689edeeed4c842b38c9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/328384
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I28b01a83960b6f7c8e715a817dc75a2408465a26
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/328658
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 has a slight ripple effect into Enum, as it was using the builtin
status as an indicator that the enum was shared with C++ code. This now
has a dedicated bool flag.
Change-Id: Id03efa902546775666acd031e6d57123e02b6c6e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/328381
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Our Metal codegen assumes that out params are pointers, but Metal's
built-in frexp actually takes a reference for the exponent, not a
pointer. We now add in a helper function to translate.
Change-Id: I24686347d07151dd99a1ff1c43aff2b35c3181e5
Bug: skia:10762
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/328387
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: Iff707ae968b84609acde89ba94dec7a5d53c7525
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/328576
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>
For posterity, here's my initial, wrong thinking:
If we squint at "return foo" and read it as "result = foo; goto
end_of_program", and then remember we can always skip forward jumps (and
where's further forward than end of program?), early returns turn out to
work just like a store.
The reason this is wrong is that by the time we reach a final return,
the entire mask stack has been popped back down to its original default
ffffffff (active) state. But that return shouldn't override any prior
returns. So that scheme isn't quite right.
Instead we accumulate the result by disabling updates to lanes that have
already returned. By the time we're done, all lanes should have hit
_some_ active return, now asserted.
Bug: skia:10852
Change-Id: I27b05f04a60ff4a5f2fe5f59bf398c3f7224a41b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/327457
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This is useful because we can clone FunctionDefinitions without cloning
the matching FunctionDeclaration. The FunctionDeclaration will remain a
builtin, but the definition should be a malleable clone.
Change-Id: Icfc1e0855fb8fcd6914a5d657f5098986fcf19ea
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/328396
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This causes a ~4% regression on sksl_large, but some of that
can be bought back in two ways:
1) Removing (now unnecessary) cloning of program elements
2) Hoisting the new analysis passes, with (nontrivial)
logic to update/maintain the call counts as we edit IR.
Also, this fixes bugs where we were emitting functions that
had "calls" from no-longer called functions.
Bug: skia:10776
Change-Id: I4f8c29957be2e4233a883c9a1125f363b82ee40c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/327198
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Previously, we'd add UnresolvedFunction symbols wherever the original
FunctionDeclaration was found, but that can alter shared SymbolTables
(e.g. the root table) and the node will not be cleaned up when the
program is deleted. The pooling system expects that any node created as
part of a Program will go away when the Program does.
This change puts the UnresolvedFunction symbol as close to the
FunctionDeclaration as possible without changing builtin symbol tables.
Change-Id: I4c80cfba734047f11b50c86829d95d8e393c3060
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/327921
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This flag is set on anything rehydrated and on the root symbol table;
that is, anything that will outlive your Program's tree.
Change-Id: Idd9a167ee69f1fb9e526aecbee733ce1ccb5d265
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/327920
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
The semantics of `vector::reserve` and `SkTArray::reserve` were not the
same. SkTArray::reserve takes a delta over the current array size,
whereas vector takes a total array size. This could lead to subtle
errors with over- or under-reservation, hurting performance.
This CL renames `SkTArray::reserve` to `SkTArray::reserve_back` to give
the SkTArray behavior a distinct (hopefully easily understandable) name,
leaving its functionality as-is.
Change-Id: Icbd3114bb317fd5f307f393c02ae6fb6f83764e9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/326956
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
Change-Id: I03bdef43c79bc3c997f9a9a6aa8fbb1a7194943a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/326437
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Now that we have skvx::map(), anyone can write this sort of
scalar-to-vector code. There are no vector instructions for these, so
they'll never going to be particularly interesting for SkVx to provide.
We did work out _approximate_ versions of each of these for SkVM, and
that's what we use to evaluate these programs there. So if this stuff
really matters we could port that logic back over to SkVx.h.
But in terms of pure refactoring, I think this is where we want to sit
until we decide to use those approximations. I don't really want to
invest much time in the SkSLByteCode interpreter any more.
Change-Id: I4e595dee5fd9e608905305e46b2aebcab986c561
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/326277
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: Mike Klein <mtklein@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This fixes up a handful of places which weren't caught in the initial
work (at http://review.skia.org/325861) because they haven't been
converted to the new IRNode structure yet.
Change-Id: I86b61fe3c601711b5802fe35218ca2e6378634da
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/326357
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I40ceb5cab0473c08c92fbb6aa9afad6173c0fb37
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/325624
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This will help us avoid allocations for arrays of statements.
Currently, this is a wash in Nanobench (~0% change). In the near
future, we expect to collapse the expression array and statement array
into a single hybrid array, and this helps bring us closer to that end
goal.
Change-Id: Id146907352799c41b568090ab65e454247b294c8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/325625
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This lets us declare it correctly depending on caps, rather than editing
the modifiers of a (shared) global variable.
Change-Id: Ifcdc5e0a8e9b852685ee03a46b537b986b5d35de
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/325623
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This will help us avoid allocations for simple expressions.
Nanobench shows ~5% improvement with an array size of 2:
http://screen/8oDEY7hjrhY8C6k
Other array sizes will show different levels of improvement, but I
haven't done an exhaustive trial. (2 was noticeably better than 1.)
Change-Id: I005a7896a0db83df4e3c2d3c0fa3321203f8a0b3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/325861
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit d34d56e279.
Reason for revert: Pinpoint
This is not a pure rollback (we keep the early-out checks for
fInlineThreshold to avoid breaking tests).
Original change's description:
> Clean up SkSL inliner and allow it to be disabled.
>
> - When fInlineThreshold is zero, the inliner doesn't need to run at all.
> - Inlining functionality outside of `analyze` can be made private now;
> it's not referenced by the IRGenerator any more.
>
> Change-Id: If61fd8998bc024201bf4489b7aa48ac4d117e449
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/325617
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I288d82273abfc2587859cc92d5a4c663694c38a4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/325621
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Adds vec[N], mat[N], and mat[NxM] aliases when building runtime effect
code. Also moves the "shader" alias for fragmentProcessor so it's only
usable in that context.
Bug: skia:10679
Change-Id: Ia337bb680c50da32639bb1d4ffc3bc01df306b0f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/325620
Auto-Submit: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
As a prelude to going back to sharing global data (safely), we want to
eliminate as much mutation of shared state as possible. The special
cases for global variable declaration were unnecessary, so just remove
them. The editing of main's parameters immediately after they were
created is also unnecessary - just hoist the logic up so we create the
variables correctly in the first place.
There is still one use, related to invocation ID. That's more
complicated (?), so leaving it as a separate CL.
Change-Id: Ia3dad78dd5a634273b2e2239368be7adaff65f38
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/325661
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Auto-Submit: Brian Osman <brianosman@google.com>
This function was originally responsible for about ~1% of compilation
time, almost entirely due to unnecessary vector allocation. The call to
`coercibleTypes` was copying the result every time due to a missing &,
and the `outParameterTypes` vector was not calling reserve before being
populated. Additionally, converted the out-parameter array to an
SkSTArray so that in the common case, we should not need to allocate at
all.
Change-Id: Iad085cf4ebc61d1ae1a92cc5f214272580ab0959
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/325862
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: Idb10db5cc4fff4f10ce84c5ae021e4c0e7bfc49b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/325662
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
This golden verifies that when the inline threshold is zero, inlining is
not performed.
Change-Id: Icad6e1faed569dd1b2469874be3b9e635ad0b9ad
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/325656
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
- When fInlineThreshold is zero, the inliner doesn't need to run at all.
- Inlining functionality outside of `analyze` can be made private now;
it's not referenced by the IRGenerator any more.
Change-Id: If61fd8998bc024201bf4489b7aa48ac4d117e449
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/325617
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This reverts commit 941fc7174f.
Reason for revert: performance now seems to be roughly equal or better
(~1%) over several trials.
Nanobench: http://screen/A8e8sojaXBgbMgF
Original change's description:
> Revert "Remove inliner from IR generation stage."
>
> This reverts commit 21d7778cb5.
>
> Reason for revert: Pinpoint absolutely hates this change
>
> Original change's description:
> > Remove inliner from IR generation stage.
> >
> > There is no need to inline code during IR generation, as the optimizer
> > can now handle this.
> >
> > Change-Id: If272bfb98e945a75ec91fb4aa026e5631ac51b5b
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/315971
> > 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>
>
> TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
>
> Change-Id: I62c235415bcdc92a088e2a7f9c3d7dbf7e1bf669
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317976
> 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: I6189806c678283188f4b67ee61e5886f88c2d6fc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324891
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This CL is conceptually a revert of http://review.skia.org/320258,
although the code has changed shape a bit since that CL was landed.
This fix was too aggressive, and can lead to functions being dead-
stripped while they still have an active reference.
Change-Id: I6ce8b0ad9cc2a42e8be8cb10d3a8219149eca6aa
Bug: skia:10776
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/325462
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: I05b940c69b7756d41277626fc3eef06003d133c1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324886
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
These aren't allowed in GLSL, and typically don't make sense.
Change-Id: I0afca0df638590466922a809e91ef0be35b13ca8
Bug: skia:10765
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324816
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
- Move all of IR generator's fields private (except for fContext, which
is used ~everywhere).
- Eliminate start() and finish(), fold this logic into convertProgram.
The division of what was set/reset in different places was pretty
arbitrary. Now, convertProgram does everything. Along that line, have
it actually return the "outputs" as an IRBundle (a small collection of
the things that the compiler needs). This seems better than the
compiler ripping out IR generator's internals.
- IR generator's POD field initialization was a mix of in-class and
constructor. Move all the constant initialization to declarations.
- No need to look up sk_PerVertex at start (or convertProgram) time, so
remove fSkPerVertex, and just do the lookup when we're about to use
it.
- IRGenerator::convertProgram is fairly long now, but all the code is in
one place. You don't have to think about the order that three
different member functions are called (along with the caller mutating
the internal state between those three calls).
- In the compiler, add an AutoSource helper to manage changing and
restoring the fSource pointer everywhere.
- Rename the loadXXXIntrinsics functions to loadXXXModule, have them
return the module, and wrap the whole thing up in a single
moduleForProgramKind() helper.
Change-Id: I0c9b6702f8786792963e3d9408d6619e5ab393e2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324696
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
We were passing in the IR generator's which had already been reset, so
technically belonged to the *next* Program to be compiled.
Change-Id: Ib68c283591f02d1642bb7c2d9658f5caa76b0f15
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324700
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This CL introduces two significant changes:
- unordered_map is replaced by SkTHashMap.
- The StringFragment key is replaced by a SymbolKey class, which caches
the hash of the string.
The second change is responsible for the lion's share of the measured
performance improvement. SymbolTable::operator[] was previously re-
hashing the string every time it recursed into the parent symbol table.
Nanobench shows a fairly consistent ~8% speed improvement in both
sksl_medium and sksl_large: http://screen/3KTc27gt85HS4wK
Change-Id: Ia1010dcd40d7f7ecdce1aa5bb10c6cf6f6f11a80
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324628
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Change-Id: Ib2fd8c246799a1bde566395080fe6617754644f9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324635
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Changed a couple of SkSL enums to enum classes and rearranged things to
make their storage within IRNode type safe.
Change-Id: I6509d027d79161c1a09473e90943aae061583f20
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324624
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Prefix, Postfix, and ExternalFunctionCall nodes this time around.
Change-Id: I56bc06d73274f01b67f043a6ebd23dd4c80d16e8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324621
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
This binds together the IntrinsicMap and SymbolTable for each include to
a single entity, with helper functions that create and return them. Used
a little bit of macro trickery to move all of the standalone/runtime
logic into loadIncludeFile, which drastically reduces boilerplate.
Change-Id: Ic70c0d67967cc614daeab5c50412ab69dcdf2fea
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324124
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This is a reland of 6bbf026b54
Original change's description:
> Add sk_Caps.builtinDeterminantSupport and use it in cross().
>
> This CL partially relands http://review.skia.org/321790.
>
> Change-Id: I26a1aefda8a01167783e6e7fa15a51aa35ee5d82
> Bug: skia:10819, skia:10810
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/323784
> Reviewed-by: Chris Dalton <csmartdalton@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
Bug: skia:10819
Bug: skia:10810
Change-Id: I7731f93db07bc917707cbbe1daca2e5ce0f763d7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324620
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Change-Id: Ib1374e1dce1a654a83813dbe341774bd91729796
Bug: skia:10694, skia:10819
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324356
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This CL also alphabetizes the various factories in ShaderCapsFactory.
Change-Id: I0378ceb821678173e72690d5563d2a9a92d90201
Bug: skia:10694, skia:10819
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324257
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit 6bbf026b54.
Reason for revert: Breaking Metal bot.
Original change's description:
> Add sk_Caps.builtinDeterminantSupport and use it in cross().
>
> This CL partially relands http://review.skia.org/321790.
>
> Change-Id: I26a1aefda8a01167783e6e7fa15a51aa35ee5d82
> Bug: skia:10819, skia:10810
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/323784
> Reviewed-by: Chris Dalton <csmartdalton@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
TBR=csmartdalton@google.com,johnstiles@google.com
Change-Id: I4a6c1a63dc38682dd965f78f0c1da98f35b6dbad
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:10819
Bug: skia:10810
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324264
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
Instead, just expose the vector directly (as elements()).
Change-Id: I9f6a3ae38cd8e6f1b0a1087a42d61452fe883924
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324130
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This improves SkSL compile times by about 1-2%.
Nanobench: http://screen/BL5kKgwzEzTNqSu
Change-Id: I85df42bdd33cfce13345f11d588283eeaf5643d0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324061
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: Ica329608e5ee7f873a55dc27e6c14bb8734665a3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324118
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
The benchmarks show a negligible performance decrease over the bespoke
TinyUnorderedMap class, but it's well within the margin of error, and
a real hash map will scale better in pathological cases.
Nanobench: http://screen/537ETJivpGdVJpk
Change-Id: I21279c47742a5dac81d57c7e9f7da4bfc595fdc9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/323114
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I97a59563914c4f75f8cfdc2bd5a9ae430de9bb3c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/323881
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
As part of this change, broke up IRNode::type() and moved it into
virtual functions where it belongs.
Change-Id: Ib19e99f2e8392e34a20a1887b980225ac4b793f7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/323884
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Change-Id: I82d4aaf442be262ad8e397de4f820831f4797b95
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/323977
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
We now support cloning global variables that are "declared" by an
InterfaceBlock, or a GlocalVarDeclaration. At this point, there are no
"inherited" elements - any mutable data that starts out owned by the
pre-includes is cloned.
Follow-up will remove the inherited element list entirely - splitting
this out to make the changes clearer.
Bug: skia:10589
Change-Id: I2a95c73bf53db313e9f3467c681a05dffffeaa3a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/323976
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This appears to just be dead code.
Change-Id: I2a8510f2e8c15a833af72552a13c2a893b3b123a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/323925
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 next pre-include refactor will require cloning InterfaceBlocks, and
re-targeting the variable.
Change-Id: Iccfc1f39789fcd572199682386cd612500334061
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/323890
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>
Change-Id: Icccc14019ae5c87a09c8439fa1b1ae324b8e32bf
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/323777
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
The goal is to have all of the IR structs be classes when the
rearchitecture is complete. Some of these should have been changed in
earlier CLs, and some of them never contained any data in the first
place and thus won't be affected by the rearchitecture.
Change-Id: Id5a3cebffd59a4c43b79f2195b50106ede7c2f87
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/323882
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
We now assume that the Symbol's name() is its actual name, instead of
having it passed it in by the caller. This simplifies calling code, and
also removes redundant symbol names from the Dehydrator, saving a few
hundred bytes here and there.
Change-Id: I3e3d65b238ea58ab52f5dca205458d6f45c06c3d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/323110
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I53af66c1b65971c204ac7c515e0d0e39481b015d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/323097
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
