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>
This reintroduces the flipped-array-dimensions bug in skia:10924. It
will be fixed in followup CLs.
Change-Id: I24ec687209b397f5fd0cf44194d0e21fe30dc32c
Bug: skia:10924
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339797
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: 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>
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>
Just filling in a gap in our tests. The output is a little strange as it
exposes a missed opportunity to constant-fold array accesses, but it
seems fine otherwise.
Change-Id: I6df13e0f9a49455015ceb47d7802bb5e1bbdaa1a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339217
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>
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>
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>
This test verifies that dead-stripping works on both built-in and user
functions, if their function call is optimized away.
Change-Id: I3125a34640c69de43c383343cd00d97e5a32ac60
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338836
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@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>
There were a surprisingly small number of dedicated SPIR-V tests.
SkSLSPIRVBadOffset was the only test that didn't already exist in the
golden outputs, although it actually contained two tests.
The SPIRVTest.cpp file has been converted to SPIRVTestbed.cpp, which can
be used for local debugging of SPIR-V issues via dm (like GLSLTestbed
and MetalTestbed).
Change-Id: I978d8a7cf5735af7f537113d2b9411ce42cfcf88
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338756
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@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>
Change-Id: Ifc1f0921d983ee09d7bc2632aeca41689f1bf0c3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338603
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 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>
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>
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>
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>
Previously, every output was labeled ".asm.frag" regardless of the
actual type.
Change-Id: Icf3a56bb04d88cc0443f12c2dfb99c66ee00dff0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/337717
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: I1be21b428939d17bbf3a9347a64db56c7cd69eb4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/337638
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 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>
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>
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>
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>
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 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>
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>
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>
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>
Metal-specific tests are pretty thin on the ground here, and some of
the remaining tests no longer added value as they were already covered
pretty well by existing tests in Shared. The majority of remaining tests
were specific to Metal's lack of flexible matrix casting (and SkSL's
ability to paper over this with helper functions).
Change-Id: I7b3c445268b95320e7f46ec88d793c315d43ee8a
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334956
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@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>
In practice, the inline threshold does a good job of limiting the
blast radius here.
Change-Id: I495184116e733262ea9d84fec30885ea047ca116
Bug: skia:10945
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334597
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@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>
At present, we do not report any error; the values wrap silently.
Change-Id: I8c435cfdd81f6c2e5fd87e9c39c708138bf4ec82
Bug: skia:10932
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/333676
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@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 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)
A followup CL will limit array dimensionality to 8. This is an arbitrary
choice which is hopefully larger than any reasonable program will need.
Change-Id: I4cf05f40ec92c1c3444c71c45f759bb30d7da3c9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/333135
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>
A GLSL function like:
void fn(int x[1][2][3]) {...}
Will emit SkSL with the array dimensions in reverse order:
void fn(int x[3][2][1]) {...}
Trying to invoke the function will fail because it expects a reverse-
dimensioned array.
Change-Id: I24431aabd2f6111b5493f63f0a85f9c78514d522
Bug: skia:10924
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/333317
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@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>
`in` vars shouldn't support initializer expressions at all. The fuzzer
noticed that dead-stripping interacts poorly with `in` var initializer
expressions, which makes sense because it's an unsupported and untested
path. In a followup CL, lines 1 and 3 will both become errors.
Change-Id: Ibb64ca319a046b040eea976acb6798a1402451de
Bug: oss-fuzz:27300
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/333128
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 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 test is meant to demonstrate that constant folding for int and
float vectors is not on equal footing. Float vectors currently generate
better-optimized output.
Change-Id: Ib4822c7b594e9bc4eb4fb9cfe6ab46f7f76268d6
Bug: skia:10908
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/332423
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@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>
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>
Change-Id: Ibc1a8d3ebbf62cc55d013f7d9146f6b155d11da2
Bug: oss-fuzz:26830, oss-fuzz:26789
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/332377
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, 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>
Change-Id: I19a9564ac4d52b709b8fdd757b99222372c626f4
Bug: oss-fuzz:26942
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/331598
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>
- Prototypes for never-declared functions
- Prototype before use
- Prototype after use
- A variety of inputs and outputs on the prototyped functions.
- Calling declared-but-undefined functions
Currently, the prototypes are not actually emitted in the generated GLSL
or Metal output at all. This CL is demonstrates our baseline before
proper prototype support is added.
Change-Id: I6112e0a89ab9bbecefccaca9fba985bb8011fff1
Bug: skia:10872
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/331376
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This improves the test output for Metal. Previously, the Metal output
was just an error message, since 1D textures were unsupported. Now we
have a valid golden output for the 2D case in Metal. (1D is still
unsupported and is likely to remain unsupported; Skia currently has no
use case for 1D textures.)
Change-Id: I91977712030f08e371cc6bfb2afa578940ca00b7
Bug: skia:10797
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330940
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@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>
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>
(This CL also adds modulo to the IntFolding shared test, since this was
absent from the test. It's implemented and working properly already.)
Change-Id: I24a947ab38754bff2624cd5b58cf7a39553ca888
Bug: skia:10870
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330596
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>
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>
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>
`func1` and `func2` emit bad code, `return %s()`, and because they don't
consume their `%s` format argument. This leaves the format argument list
unbalanced and all future args are wrong.
Another serious problem is that we don't actually know the names of the
functions that they need to call, because we haven't emitted them yet.
`func3` is not emitted at all. Sampling from a fragment processor
apparently fails in this context.
This is a more general case repro for skia:10684--it turns out that
recursion in particular wasn't the issue, but nested function calls just
don't work properly at all in FP files. This wasn't an issue in practice
because we don't have any existing FP files which nest function calls,
and the inliner also tends to aggressively flatten everything out if we
don't explicitly disable it.
Change-Id: Iff029c459c7d90be566f9b4c9be0e3150e459866
Bug: skia:10684
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329367
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
The generated code does not assign to sk_OutColor correctly; it assigns
into the `factorial` function name instead, which doesn't make sense.
Change-Id: Ibad1d47f2f9c4fbb410b5277cea6e1022daf8b9d
Bug: skia:10684
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329360
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 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>
These don't compile in GLSL, so they shouldn't compile in SkSL either--
and fortunately, they do not.
(In C++, and consequently in Metal, these expressions are considered
legal by the grammar and do compile, but generate garbage output.)
Change-Id: I6c7bea70b3d91677ccd8fcbad1eba123d655e856
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329359
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
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>
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>
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>
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>
Declaring max_vertices before invocations fails to adjust max_vertices
when invocation support is not present. (It should be 4, not 2 in this
case).
Bug: skia:10827
Change-Id: Ief7af97eabf5414ea8363808fc1ad2e9c480fe10
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/325664
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@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>
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>
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>
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>
This removes VarDeclarationsStatement entirely. VarDeclaration instances
appear directly as statements in Programs. SkSL that declares multiple
variables in a single declaration is transformed to represent that as a
series of VarDeclaration statements.
Similarly, global variable declarations are represented by
GlobalVarDeclaration program elements, one per variable.
Bug: skia:10806
Change-Id: Idd8a2d971a8217733ed57f0dd2249d62f2f0e9c5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/323102
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Just a typo fix.
Change-Id: I2fe1f6ae1c99d7f20a4fa5f49eefea514e224652
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/321977
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 greatly improves the output from a profiler. It makes it much
easier to determine how much time is spent in searching for candidates,
versus actually inlining them.
It also improves the code readability somewhat by breaking a large
monolithic function into several smaller functions.
Change-Id: I1b3ef6ddbe46af60e673f37ded766f8077ed6b03
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/321376
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
We were letting this get further, then asserting.
Bug: skia:10797
Change-Id: Iff6fe43aa32450b5a517c94773031d593f1f62a2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/321794
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
We don't need to create a temporary variable for expressions like
`half3(x)`.
Change-Id: Ie0fa6a6dfb3d77d4372f96c676d3081f7e278852
Bug: skia:10786
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/320960
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
For instance, `foo[0].x` is now considered trivial to inline. It
combines two trivial cases: array-indexing by an int literal, and a
swizzle.
Change-Id: Ibb3ca1f324bbee0e9b3556e66644923fc9e0cf45
Bug: skia:10786
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/320768
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
The following types of expression are hoisted directly into the
inlined code:
- Struct field access: `myStruct.myField`
- Swizzles: `myVector.xzy`
- Simple array indexes: `myArray[0]`
This significantly reduces the number of temporary variables generated
by the inliner.
Change-Id: Ifed226ecc87b096ec1e38752c0c38ae32bd31578
Bug: skia:10737, skia:10786
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319919
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
The right side of assignments will collapse redundant swizzles, but the
left side does not. Code like this can actually cause the ByteCode-
Generator to assert if it is run (it asserts in `swizzle_is_simple`
during code generation).
Change-Id: I891912fe0b5de2670dfa95f6702a86d5c42bb2ec
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/320296
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This was adapted from a test in SkSLInterpreterOutParams and presents a
challenging double swizzle.
Change-Id: Icb7b3bbb18d4b3cfa0c26acb524c08812ba88096
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319920
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 allows dead-stripping to properly optimize away unreferenced clones
of intrinsic functions, and allows the inliner to detect intrinsic
functions that are only called once (which can generally always be
inlined without penalty).
Change-Id: I0cf034d880ae5d52f4cc0f93de6e2c7aad34e975
Bug: skia:10776
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/320258
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 reland of 435b482638
inlineStatement now takes a `const Expression* resultExpr` instead of
`const Expression& resultExpr` because resultExpr will be null for a
void function.
Original change's description:
> Support out parameters that use a swizzle.
>
> This CL also removes the `VariableExpression` class that was briefly
> added in a prior CL. This class was intended to support cloning an
> expression while changing the refKind of a VariableReference inside of
> the expression, but it added state and complexity. In this CL, rather
> than track this via extra state, the inliner just recurses into the
> expression as needed to find its VariableReference. Since most relevant
> expressions are just a VariableReference anyway, this is inexpensive.
>
> Change-Id: Id4d926b7d7520b5e6ce455446c05a6d59ef62a84
> Bug: skia:10756
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319917
> Commit-Queue: John Stiles <johnstiles@google.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
Bug: skia:10756
Change-Id: I35f76c21eccf0ba2ab47e4313e131f7aa26980fa
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/320223
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This reverts commit 435b482638.
Reason for revert: ASAN/UBSAN unhappy.
Original change's description:
> Support out parameters that use a swizzle.
>
> This CL also removes the `VariableExpression` class that was briefly
> added in a prior CL. This class was intended to support cloning an
> expression while changing the refKind of a VariableReference inside of
> the expression, but it added state and complexity. In this CL, rather
> than track this via extra state, the inliner just recurses into the
> expression as needed to find its VariableReference. Since most relevant
> expressions are just a VariableReference anyway, this is inexpensive.
>
> Change-Id: Id4d926b7d7520b5e6ce455446c05a6d59ef62a84
> Bug: skia:10756
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319917
> Commit-Queue: John Stiles <johnstiles@google.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: Ibdda47607f9e6e7f3a7459915067cf5e20919993
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:10756
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/320220
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This CL also removes the `VariableExpression` class that was briefly
added in a prior CL. This class was intended to support cloning an
expression while changing the refKind of a VariableReference inside of
the expression, but it added state and complexity. In this CL, rather
than track this via extra state, the inliner just recurses into the
expression as needed to find its VariableReference. Since most relevant
expressions are just a VariableReference anyway, this is inexpensive.
Change-Id: Id4d926b7d7520b5e6ce455446c05a6d59ef62a84
Bug: skia:10756
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319917
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Swizzles in combination with out params are currently broken in SkSL.
They are fixed in the followup CL at http://review.skia.org/319917
Change-Id: I22d9436a15631e6ee2acf9fc312a8634cf3b5407
Bug: skia:10756
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319918
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>
GLSL does not support assigning to ternaries, and will fail to compile
and/or generate non-functional shaders if we pass in a shader that tries
to assign into a ternary expression.
If SkSL is able to completely eliminate the ternary (e.g. if it boils
down to a simple `true ? x : y` or `false ? x : y`), SkSL can strip out
the ternary entirely and generate valid GLSL. This case is harmless and
so it is still allowed.
Change-Id: I960f119fb9934f998697634e6c4e519cd77d3780
Bug: skia:10767
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319679
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I452e52a87d89cefb5c21a0d9d57e9771f3038d73
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319783
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
This was unused and did not work on non-GLSL backends.
Change-Id: I6bd314d43cfefa64871b5c0e964b5ae52e494164
Bug: skia:10757
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319778
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Chris Dalton <csmartdalton@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Chris Dalton <csmartdalton@google.com>
This will allow the inliner to use IsAssignable.
Change-Id: Ic94f71002779b53d0b3dc97f37fbe4bb98b026d8
Bug: skia:10756
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319414
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
These tests will also be used for Metal and SPIR-V testing. A small
handful of GLSL-specific stragglers (#version-specific or type-precision
related) will remain in /glsl/.
Change-Id: I7f2b2bd92825c327922c8ce74e438d2daa440dff
Bug: skia:10649
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319408
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Many calls to `setRefKind` failed to check the return value; if it's
false, an error has occurred and the program is in a bad state.
Specifically, there is an assignment to a variable that's not marked as
"written-to." If we continue processing the program, we're likely to
assert.
Change-Id: I2dd5d1f41aa5ca0d30f8d638f05fe2e838216d78
Bug: skia:10753
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319116
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
These cover:
- Properly configured out-params
- Invalid/non-lvalue out-params, which currently cause an SkSL crash
- Interactions between the inliner and variable swizzles
Change-Id: I4874101236084f273e704d8717149b431d813883
Bug: skia:10753
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319036
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
(Removed one test, SkSLFPSwitchWithMultipleReturnsInside, because it was
redundant with existing tests.)
Change-Id: I1bfc069babdb5eb0cc515f195c3a2e307bb5871a
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319029
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>
Cleaned up some nearby code while implementing this fix as well.
Change-Id: Ic084451f0d9fc12169e1720a8889a290249eb5e9
Bug: skia:10750
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/318796
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 copied intrinsic functions in a totally arbitrary order;
it used an unordered_set of pointers, so it could be affected by
switching standard libraries OR by malloc nondeterminism. (Surprisingly,
it was fairly consistent in practice on OS X/Linux.) This CL sorts the
intrinsic functions into a consistent order before copying them.
Change-Id: If90342bb77a9ae237a3ce91be3a9847311a722c4
Bug: skia:10749
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/318700
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Code like
bool4 result = val.xy01;
Will now be converted to:
bvec4 result = bvec4(val.xy, bool(0), bool(1));
Previously it tried to do this, but there isn't an implicit conversion
from int to bool, so it was silently failing and adding nulls into the
constructor:
bvec4 result = bvec4(val.xy, $coerceToBool(0), $coerceToBool(1));
This CL also cleans up some related code that I was checking while
trying to understand the nature of the error.
Change-Id: I5b7d96760a03170ff78b46251c4182cc4e89836f
Bug: oss-fuzz:25781
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/318636
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This test currently crashes skslc, but will be fixed in the followup CL.
Change-Id: I3683d94a310242e8ca67560296518fd1223b28d0
Bug: oss-fuzz:25781
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/318656
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Ideally the inliner would be smart enough to avoid creating a temporary
at all just for a swizzle, but a good first step is to create fewer of
them.
Change-Id: Icd6f86c294237488f7923dc787bb64a5f99bd0ac
Bug: skia:10737
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/318213
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Early returns can cause the inliner to generate suboptimal code. We
control our built-ins, so let's avoid them where we can.
Patterns like this challenge the inliner:
if (x) return y;
return z;
But this can be replaced by equivalent code that inlines better:
return x ? y : z;
Or, if a ternary can't be used, this also does a better job:
if (x) return y;
else return z;
In several cases, this allows the inliner to avoid generating a
do-while(false) block for control flow.
Change-Id: I921c929122297c40476ff15b4da631fc1581e308
Bug: skia:10737
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/318211
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
I realized that "DefaultSettings" as a name suffix was unclear, because
"Default" is a different settings mode from skslc running with
--nosettings.
In --nosettings mode, skslc uses "standalone" settings.
Change-Id: I1f5d80df0a21cec55948c4ad146169bcb34f4999
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/318210
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Several blend functions generate a surprisingly large amount of code,
and appear to have opportunities for further optimization. At any rate,
if we make compiler changes that would affect the output of a blend
function, I think we would want to see the changes reflected in our
golden outputs.
Change-Id: Iff612dcd4bad8824b5e6e97413ffce19e5ea1c0e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/318336
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This wasn't rolled back automatically by the revert at
http://review.skia.org/317976, because this unit test did not exist yet
when that CL was submitted.
Change-Id: Ib887e74ddd32c1e576908bbe3d588205e26cdaa5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317978
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit 910845fac1.
Reason for revert: IRGenerator inline change reverted
Original change's description:
> Add program-settings flag to disable the inliner.
>
> Change-Id: I6c4e7f6a2aab6710221029022a3a5f3ec323c5e2
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317856
> 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: Ie38a29495ea8497f9db26d2603df179e696ac5ff
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317977
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
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>
This resolves the following TODO block:
TODO(johnstiles): the skslc standalone caps bits do not enable
do-while support, so this test does not actually perform as
described; the `returny` function is not inlined at all. This will
be fixed when customizable caps-bit support is added to the golden
tests.
Change-Id: I3495e4813b9be37264a8fda978453594c1f5fa13
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317859
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Ideally the optimizer should be able to detect and remove this loop.
This CL establishes a baseline.
Change-Id: I6aba0b52fe49552f170fca25d81c29c515044ef5
Bug: skia:10737
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317861
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: I6c4e7f6a2aab6710221029022a3a5f3ec323c5e2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317856
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I72fd3083f75ca5bf74fb2c3b032465864a13aed5
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317771
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>
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>
Change-Id: I01c150d6bfcdd1500033521a87c058c7428c3521
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317769
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Change-Id: I35ff25c4cc394c1a4a964207ece87095a9ba84cf
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317767
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: Ic8f4730d035981c32b4ddb48e5e919b0396b6d93
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317578
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
We were allowing these ops on floating point types. The resulting GLSL
would fail to compile. We also allowed >>= and <<= on vectors (but not
any of the other bitwise ops).
The newly added unit tests were failing (eg, not catching errors). New
results are correct.
Bug: skia:10707
Change-Id: I97b769f1ce59261361109a71061b42dc8ef3c74b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317393
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Change-Id: I83a38f2c953a560fea3483e95e31df532b90773e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317456
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
We now support building an SkSL golden output twice, once honoring the
custom #pragma settings, and once more ignoring the settings. This
allows us to see the output of the workaround technique, alongside the
"default-settings" output which should not contain a workaround.
To implement this, skslc now supports a flag: --[no]settings.
When it's set, /*#pragma settings*/ comments are honored. When it's not
set, skslc ignores the comments. compile_sksl_tests.py passes this flag
along to skslc.
This approach is not strictly limited to workarounds; the
"TypePrecision" GLSL test was also updated to use this technique.
Change-Id: I79204df047b024533ed6bc1f4c088e0e878d5bb1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317246
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This will allow us to write skslc-based golden tests that deviate from
the standalone skslc settings.
This CL provides six options; more will be added as necessary.
- Default (caps)
- UsesPrecisionModifiers (caps)
- Version110 (caps)
- Version450Core (caps)
- ForceHighPrecision (settings flag bit)
- Sharpen (settings flag bit)
Change-Id: I9b779e039b2f0c0ccf43dca226fb4844aff3526b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317237
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
A handful of simplifications were made, but these hew very close to the
original tests and are intended to cover the exact same ground. The
remaining unconverted tests depend on non-default caps bits and will
be updated once caps handling in skslc is fully landed.
Change-Id: I3f3c29bf87f73e501561d7bfcdaabe8acc14b89f
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317390
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Bug: skia:10721
Change-Id: Ib9e51b736bafb6e4493db4f50e9835fbd86c415d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317247
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This simplifies life when revising the swizzle logic.
Change-Id: I7fc63c0cc845c4741c17c82b3078040264b61ba0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317379
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>
`fBuiltinFMASupport` is now true on both, and
`fUsesPrecisionModifiers` is now false. Other mismatching flags exist,
but they are non-trivial to synchronize as they are tied to extension
strings.
This will help our skslc-based unit tests generate the same results as
our C++ unit tests did, but should not affect real-world results as
these defaults will all be overwritten in a non-testing scenario.
In practice, the `fUsesPrecisionModifiers` change is responsible for all
of the diffs below. The other flags did not change the results of any of
the currently-ported tests.
Change-Id: Ieb056d852b027fa87c56fd89f971a77a10a8a124
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317204
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
GN lists both the .cpp and the .h as generated outputs, so if they don't
exist, Ninja assumes we need to rebuild the tests every time we compile.
Change-Id: I37b8b3d9e7aef1b0cb8d5c70530c2542a6c0087a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317108
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 lack of proper caps-bits controls in skslc affects the outcome of
one test: "InlinerWrapsEarlyReturnsWithDoWhileBlock" does not actually
emit the do-while block because the standalone caps bits don't enable
do-while support. This will be fixed in a followup CL that adds caps-bit
support to our tests.
A few tests were renamed for consistency, a few were simplified slightly
and one test was removed because it was simply redundant (there was a
second test that covered the exact same ground as
`ForWithReturnInsideCannotBeInlined`).
Change-Id: I2e3b97cb3aea331b6d806bdb865aa78c35c7a6b9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/316997
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Change-Id: I6097f50e7be1fa2f7772f6c454410ecbf3470eea
Bug: skia:10722
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/316977
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
The following conditions lead to the error:
- A pair of nested functions, both of which must be inlined.
- Both inlined functions create a variable with the same name.
- The outer function passes its variable to the inner function.
- The initialization of the inner variable uses the value from the outer
variable.
- The inner function does not mutate the variable, use it as an out-
parameter, or otherwise cause it to receive a temporary copy.
When all these conditions are met, both variable declarations are
inlined as-is without performing any name salting, because it's
seemingly safe to do so. The name overlap issue is not considered in the
safety checks. Inlined variable declarations are not subject to name
salting but they should be; I suspect other adversarial examples could
be crafted as well where unhandled name overlap leads to errors.
Change-Id: Ia754bee8e45c8a5c7548436594bbf04abc7a8396
Bug: skia:10722
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/316945
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
The new test files are intended to be identical to the unit tests in
every meaningful way. (Comments and formatting are not preserved
exactly.) In cases where a unit-test method contained more than one
test, multiple test files were created; in these cases, new names were
invented to match the apparent intent of each invocation.
Followup CLs will continue to migrate additional tests.
Change-Id: I785c6761ba7ee2b25b5ddc0108321734be23b77c
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/316678
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I56e19153597e2c4393c5821314b82937828c0d50
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/316569
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Golden SkSL outputs are intended to eventually replace the majority of
our unit tests, since they can automatically update themselves when we
change implementation details of the compiler.
If you change the compiler output without updating the Golden files, the
CheckGeneratedFiles housekeeper will be triggered. Set
`skia_compile_processors` or `skia_compile_sksl_tests` to true in your
GN args to regenerate them.
Almost all of the tests from SkSLFPTests.cpp and SkSLGLSLTests.cpp can
be migrated into separate unit-test .fp/.sksl files in a followup CL.
hcm@ has signed off on removing the copyright boilerplate preamble from
our unit test files.
Change-Id: I9e24a944bbac8f8efd62c92481b022a0b1ecdd0b
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/316336
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
