Commit Graph

478 Commits

Author SHA1 Message Date
John Stiles
3ed2981da2 Update test to demonstrate out-of-range value in error.
SkSL is somehow interpreting a large positive value as a negative one.

Change-Id: I299e0bf389a9fcbfe697741bd33a54df07748753
Bug: skia:12863
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/499556
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2022-01-25 17:23:48 +00:00
John Stiles
cc473cd92f Fix fuzzer-discovered error with swizzles.
Some paths through swizzle optimization would replace a swizzles with a
constructor--e.g. `float3(1, 2, 3).y` would be replaced with `float(2)`.
(Constructor::Convert was responsible for replacing this trivial
constructor with the literal `2.0`.)

The optimization code asserted that this replacement would succeed, but
the fuzzer managed to construct a counterexample where the constructor
rejected the value. Specifically, by nesting casts between int3 and
float3, it found a case where Constructor::Convert returned null because
the literal value was out of range for `int` types.

This assertion didn't really add value so removing it was harmless.
Constructor::Convert already reports an error when it fails, and null
returns are handled properly throughout.

Change-Id: I575d441ed90d6b696f6399941c3f6d84698794bc
Bug: oss-fuzz:44045
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/499382
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2022-01-25 16:25:09 +00:00
John Stiles
9f681e6df8 Reject $ in variable names for non-builtin code.
These identifiers are reserved for SkSL internal use (and can't be
exposed to GLSL or Metal anyway).

Change-Id: Id554cbf21ed2fb66785e77700ff79424ecdf66db
Bug: skia:12854
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/498036
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2022-01-24 14:17:36 +00:00
Julia Lavrova
55c215cfb6 Reland "Better Matrix/Scalar testing"
This reverts commit 455e580b9c.

Reason for revert: Fixing the build break

Original change's description:
> Revert "Better Matrix/Scalar testing"
>
> This reverts commit abb611550e.
>
> Reason for revert: Build break
> Original change's description:
> > Better Matrix/Scalar testing
> >
> > Adding tests for matrix math and comparison
> > bug: skia:12681
> >
> > Change-Id: Ia1537ee2e411383749456fd6ff938b7c9a2e1061
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/493416
> > Reviewed-by: John Stiles <johnstiles@google.com>
> > Commit-Queue: Julia Lavrova <jlavrova@google.com>
>
> Change-Id: I70871b4b75c1f10e870dc5e884a42405a80fc0f9
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/494816
> Reviewed-by: John Stiles <johnstiles@google.com>
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Commit-Queue: Julia Lavrova <jlavrova@google.com>

Change-Id: Idef8dbcd6f5a5bbe84d3fd86888e3eab0f0521ee
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/494817
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Julia Lavrova <jlavrova@google.com>
2022-01-14 19:12:00 +00:00
Julia Lavrova
455e580b9c Revert "Better Matrix/Scalar testing"
This reverts commit abb611550e.

Reason for revert: Build break
Original change's description:
> Better Matrix/Scalar testing
>
> Adding tests for matrix math and comparison
> bug: skia:12681
>
> Change-Id: Ia1537ee2e411383749456fd6ff938b7c9a2e1061
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/493416
> Reviewed-by: John Stiles <johnstiles@google.com>
> Commit-Queue: Julia Lavrova <jlavrova@google.com>

Change-Id: I70871b4b75c1f10e870dc5e884a42405a80fc0f9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/494816
Reviewed-by: John Stiles <johnstiles@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Julia Lavrova <jlavrova@google.com>
2022-01-13 18:52:04 +00:00
Julia Lavrova
abb611550e Better Matrix/Scalar testing
Adding tests for matrix math and comparison
bug: skia:12681

Change-Id: Ia1537ee2e411383749456fd6ff938b7c9a2e1061
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/493416
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Julia Lavrova <jlavrova@google.com>
2022-01-13 15:35:12 +00:00
John Stiles
866db186fa Simplify control flow in switch test to unbreak iOS.
This looks like the GLSL driver in iOS generates wrong results when
returning a value from inside a switch.

Change-Id: I478a045c64c3dae9824f86f52e0c7f8f9685c9af
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/494476
Reviewed-by: Joe Gregorio <jcgregorio@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2022-01-13 14:49:06 +00:00
John Stiles
8522bf910a Add test for switch-case folding.
Change-Id: I3dabd77890a73ea054bb57d466a6ed8273eae3e8
Bug: skia:12811
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/494196
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2022-01-12 22:24:55 +00:00
John Stiles
44c00ae64a Add test for vector constant folding.
Change-Id: Iecf1313af5f2938cb899f2a3e750ffc04554bae0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/493977
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
2022-01-12 17:23:20 +00:00
Julia Lavrova
a856f40086 Interface blocks no longer allow duplicate fields
bug: skia:12793
Change-Id: Idccdab5ee8f1c7792bdfe98efd379d0199a65377
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/492397
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Julia Lavrova <jlavrova@google.com>
2022-01-07 16:44:08 +00:00
Julia Lavrova
ae5f86ea1a Test: Interface blocks allow duplicate fields
bug: skia:12793
Change-Id: Ie47b1df381779ed24cab8b05b1ee6947bda82516
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/492396
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Julia Lavrova <jlavrova@google.com>
2022-01-07 15:55:58 +00:00
Julia Lavrova
ce49ff6520 Test: duplicate fields in the same struct allowed
As @johnstiles suggested I add the test first and the fix after.
bug: skia:12712

Change-Id: I9316cf40f71e756fc1730ee630bc0d0377f200d6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/491936
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Julia Lavrova <jlavrova@google.com>
2022-01-06 19:40:50 +00:00
John Stiles
3c233c7f73 Update comments in FirstClassArrays test.
We used to reject ES3-style array declarations in strict-ES2 mode, so
this test originally expected two errors.

Change-Id: I17f71630076cda4b37b7723225dcff951eba9dcc
Bug: skia:12410
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/491997
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
2022-01-06 18:53:34 +00:00
John Stiles
a5d4541ae5 Confirm that GLSL codegen always emits array-size after the identifier.
GLSL ES2 doesn't support the ES3-style `type[size] name;` syntax. SkSL
always emits array decls in ES2-compatible syntax, regardless of the
ordering in the input code.

Change-Id: Ibf591713b2d506db7d63a6db8b79a3246f98c3cc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/491976
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
2022-01-06 17:11:19 +00:00
Ethan Nicholas
4cd3ae4009 Revert "Fixed SkSL error reporting on array types"
This reverts commit 6e686b8b8b.

Reason for revert: After internal discussion, we established that nobody was actually sure why this had needed to be an error in the old parser in the first place, so there does not appear to be a reason to carry the behavior forward.

Original change's description:
> Fixed SkSL error reporting on array types
>
> The DSLParser was not reporting errors when the array type appeared
> before the variable name (float[2] x) as opposed to after (float x[2])
> in strict ES2 mode.
>
> Bug: skia:12410
>
> Change-Id: Ia388aa150f65916dc3ccc58f7680dbde0a636c5f
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/491819
> Reviewed-by: Brian Osman <brianosman@google.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>

Bug: skia:12410
Change-Id: I355fd1ad89e2e64b0377be7672b7f3f824eebac8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/491996
Auto-Submit: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
2022-01-06 16:53:17 +00:00
Ethan Nicholas
6e686b8b8b Fixed SkSL error reporting on array types
The DSLParser was not reporting errors when the array type appeared
before the variable name (float[2] x) as opposed to after (float x[2])
in strict ES2 mode.

Bug: skia:12410

Change-Id: Ia388aa150f65916dc3ccc58f7680dbde0a636c5f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/491819
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
2022-01-06 16:28:47 +00:00
Ethan Nicholas
f393067c37 Made SkSL type aliases into first-class objects
Previously, type aliases ('vec2') were just an additional name which
could be used to refer to a type ('float2'). This was simple and worked,
except that error messages would be wrong - any type-related error
message would refer to the type as 'float2' rather than the 'vec2' that
the user actually typed.

This CL adds an AliasType class so that we can track which name was
used to refer to an aliased type and report messages using the correct
type name.

Bug: skia:12737

Change-Id: I40e234239ab47557033e0695e4fbbd5f01da354e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/490256
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
2022-01-05 16:20:19 +00:00
Brian Osman
735ff421bb Reject #extension in runtime-effect mode
Bug: oss-fuzz:43062
Change-Id: I10d8fa40c81c5b1595d30221d89c84f5cc3478fd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/490857
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2022-01-04 16:12:50 +00:00
John Stiles
aa09c782a8 Add test for structs/interface-blocks with invalid member names.
Structs already handled this appropriately, but interface blocks did not
guard against naming their member variables built-in type names like
"float" or "bool".

Change-Id: I12ec054b3f158b83e35031449cf2a088ff8d0dc2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/489596
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
2021-12-28 20:55:32 +00:00
John Stiles
1c7d442b52 Add test containing anonymous function parameters.
SkSL will reject ES2-compatible code because function parameters always
require a name in SkSL. (A followup CL relaxes this restriction and
allows anonymous parameters in SkSL.)

Change-Id: Ifdcf0fcbe0f52d16007c018b545631ca4033a8c4
Bug: skia:12769
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/489537
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
2021-12-28 20:08:56 +00:00
John Stiles
48432e133e Add test demonstrating struct/interface-block name conflict.
Structs and interface blocks allow a trailing identifier which is added
to the symbol table. This identifier should be prohibited from
overlapping built-in types; at present, this is not checked. Add a test
demonstrating the issue.

Change-Id: I99aa915c1715c468cc369c97b7f12e031b86ea4a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/489496
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2021-12-28 19:26:17 +00:00
Brian Osman
fde20db7ca Rename SkSL's 2D cross product builtin function
Note that the 2D cross product isn't defined. There are at least two
possible interpretations of what that might mean. This name makes it
clearer that we're asking for the length of the resulting vector, if
we computed the 3D cross product (assuming Z == 0 for both vectors).

It also eliminates name overlap between builtin functions and actual
intrinsics.

Change-Id: I24e8bc0ab2ec91aaace20f0dd3e8565c10bd44a0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/484440
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
2021-12-14 22:00:26 +00:00
John Stiles
83b5237b53 Fix assertion when debug-tracing a void-return function.
The debug-slot code didn't expect to encounter a void type.

Change-Id: Ied452b51e1cf90a0c0bc24770f82e711105b8e82
Bug: skia:12708
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/482461
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
2021-12-10 14:39:05 +00:00
Brian Osman
8c3d183cc2 Rename SkSL 'srgb_unpremul' to just 'color'
Added comments to explain the semantics (both what's expected when you
set the uniform, and what you see in the shader). The old name was
confusing, because it sounded like you got an sRGB color in the shader.
This is terse, but I think it's the cleanest syntax - and for embedding
clients, they can use C++ (etc.) API to require that color uniforms are
assigned from color types.

Bug: skia:10479
Change-Id: If00ea754060494aaa83001a5b357687953de8a5f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/480577
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Derek Sollenberger <djsollen@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
2021-12-07 17:56:27 +00:00
Julia Lavrova
94f726ae62 Adding test files demonstrating type confusion for arrays/structs.
Change-Id: Iacd924471971c3c551648f75c1ecf0f1bfd3ed07
Bug: skia:12642
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/479061
Commit-Queue: Julia Lavrova <jlavrova@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
2021-12-03 20:54:36 +00:00
John Stiles
d7c3b21b79 Revert "Enable various switch tests in ES2 mode."
This reverts commit ffe365eb4d.

Reason for revert: crash inside IntelHD405 Vulkan driver
http://screen/6ugVDdjJpqDkxX6

Original change's description:
> Enable various switch tests in ES2 mode.
>
> switch is no longer an ES3-specific feature.
>
> Change-Id: Ic878a77268e517e17699c2e35a37da6b0a7765dd
> Bug: skia:12450
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/452320
> Auto-Submit: John Stiles <johnstiles@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>

Bug: skia:12450
Change-Id: Id6f32a084f1bc7b2b3a1e5fb0b82d2011e4ba780
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/479059
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
2021-12-02 19:03:11 +00:00
John Stiles
ffe365eb4d Enable various switch tests in ES2 mode.
switch is no longer an ES3-specific feature.

Change-Id: Ic878a77268e517e17699c2e35a37da6b0a7765dd
Bug: skia:12450
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/452320
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
2021-12-02 17:17:43 +00:00
John Stiles
dfe33f49f5 Add a setting to disable SkVM variable traces.
This will allow the visualizer to see line-number information without
incurring the code bloat caused by trace_var.

For reference, Commutative.skvm with no debug traces:
https://osscs.corp.google.com/skia/skia/+/main:tests/sksl/runtime/Commutative.skvm;drc=c809e5ba9fc9bc67291d088c448ed0057371b760

Change-Id: If98b5e102571d7fe06a1653d199e3ae035b3cb78
Bug: skia:12692
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/478417
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Julia Lavrova <jlavrova@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2021-12-01 17:25:59 +00:00
John Stiles
030f8260a7 Add debug traces to an existing test.
This demonstrates how much additional non-trace code can be generated
when debug traces are turned on. A followup CL adds a setting to disable
`trace_var`, which eliminates a significant percentage of the additional
ops (at the cost of removing valuable debug info).

Change-Id: I238e28e6f6531f1dbccfef8f1dcd24a1e8481669
Bug: skia:12692
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/478416
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Julia Lavrova <jlavrova@google.com>
2021-12-01 17:25:59 +00:00
Brian Osman
00aa1a9259 Prevent 'binding' and 'set' on struct/interface block fields
I should have realized the fuzzer would find this assert when I added
it. Now the front-end rejects these layout qualifiers on both struct
fields and interface block fields. LayoutInInterfaceBlock.sksl is a
reformatted version of the fuzzer input. LayoutInStruct is hand-crafted
to trigger the same failure on a different code path. Both would
previously assert in the SPIRV generator. Now, neither one gets that
far.

Bug: oss-fuzz:41347
Change-Id: Iff69d8f5482da7b772e9331c4fd2d58e89813c46
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/476396
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
2021-11-29 15:53:57 +00:00
Brian Osman
e051cd35e4 Disallow 'binding' and 'set' on push constants
These aren't allowed on push constants (it's a validation error now), so
we at least catch it in the SPIRV generator and emit an error. Fixed two
places where we were breaking this rule when automatically adjusting
layouts for interface blocks.

Bug: skia:12670
Change-Id: I08b88f4c2844da77239207817f49510118be4e60
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/474976
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
2021-11-24 19:27:14 +00:00
Brian Osman
10c77dbce3 Reland "Restrict where 'binding' and 'set' can appear"
This is a reland of 9372ef0228

Original change's description:
> Restrict where 'binding' and 'set' can appear
>
> In SPIRV, these are an error when applied to struct members. Some of our
> tests were triggering that because we had free-floating uniforms
> decorated this way (and we coalesce those into members of an interface
> block).
>
> Now, we only allow those layout qualifiers on variable types that will
> remain top-level constructs in the back-end.
>
> Bug: skia:12670
> Change-Id: I73e69cecf6237a1c1180ad38d9b5d52ea80316fb
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/474218
> Commit-Queue: Brian Osman <brianosman@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>

Bug: skia:12670
Change-Id: I01c0323bba7ce0bddea5f9fb907e2b60e6b812d2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/475156
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Brian Osman <brianosman@google.com>
2021-11-23 18:03:24 +00:00
Brian Osman
d7ebf8604e Revert "Restrict where 'binding' and 'set' can appear"
This reverts commit 9372ef0228.

Reason for revert: Unhappy bots

Original change's description:
> Restrict where 'binding' and 'set' can appear
>
> In SPIRV, these are an error when applied to struct members. Some of our
> tests were triggering that because we had free-floating uniforms
> decorated this way (and we coalesce those into members of an interface
> block).
>
> Now, we only allow those layout qualifiers on variable types that will
> remain top-level constructs in the back-end.
>
> Bug: skia:12670
> Change-Id: I73e69cecf6237a1c1180ad38d9b5d52ea80316fb
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/474218
> Commit-Queue: Brian Osman <brianosman@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>

Bug: skia:12670
Change-Id: Ie518192d9a52fc896e615ec08ce0674ad683ec61
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/475099
Auto-Submit: Brian Osman <brianosman@google.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
2021-11-22 19:52:23 +00:00
Brian Osman
9372ef0228 Restrict where 'binding' and 'set' can appear
In SPIRV, these are an error when applied to struct members. Some of our
tests were triggering that because we had free-floating uniforms
decorated this way (and we coalesce those into members of an interface
block).

Now, we only allow those layout qualifiers on variable types that will
remain top-level constructs in the back-end.

Bug: skia:12670
Change-Id: I73e69cecf6237a1c1180ad38d9b5d52ea80316fb
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/474218
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
2021-11-22 18:28:41 +00:00
Brian Osman
f72c919a9a Roll SPIRV-Headers and SPIRV-Tools
This adds new validation rules that we were breaking.
Binding and DescriptorSet can't be applied to push constants, nor to
struct members.

Bug: skia:12670
Bug: chromium:1270328
Change-Id: I332f77717b08d9945c8e5b79c5bf649a8f5f2043
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/474056
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
2021-11-19 19:22:54 +00:00
John Stiles
626dbe195a Add test for commutative operations in SkVM.
SkVM should be able to optimize away these equivalent expressions:

	(intA & intB)   <->  (intB & intA)
	(intA ^ intB)   <->  (intB ^ intA)
	(intA | intB)   <->  (intB | intA)
	(intA + intB)   <->  (intB + intA)
	(intA * intB)   <->  (intB * intA)
	(intA == intB)  <->  (intB == intA)
	(intA != intB)  <->  (intB != intA)

These should be guaranteed by IEEE754 as well:

	(floatA + floatB)   <->  (floatB + floatA)
	(floatA * floatB)   <->  (floatB * floatA)

I've added a test to demonstrate existing behavior, which leaves these
optimizations on the table.

Change-Id: I01ce1d6f1cfadb3d77db405a83752c9dd52c99bd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/473238
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
2021-11-18 21:16:48 +00:00
John Stiles
bcd6d4901d Add test for construction of void.
This fails exactly as it should, but we had no test for it.

Change-Id: I0aa3307c444f2c9bc3512ff43b784a56a7c09856
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/472449
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
2021-11-17 16:04:35 +00:00
John Stiles
85f4226bd3 Fix fuzzer-discovered error with child calls.
The `eval` methods take a shader/blender/colorFilter, and we assumed
when assembling the ChildCall expression that the child expression would
be a VariableReference because opaque objects don't participate in
normal expressions. However, comma-expressions were allowed to contain
opaque types. GLSL doesn't allow opaque types in comma-expressions:

http://screen/8YW59tYDUbBh9eW

Now we disallow them as well.

Change-Id: Iaf88ef7bddb5cc8f1f1e23b515174dfc291e00c7
Bug: oss-fuzz:41072
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/472446
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2021-11-17 15:44:21 +00:00
John Stiles
504d57e9bd Add test for void type in struct.
Mysteriously, I had written a test which put arrays of void inside a
struct, but had neglected to include the non-array case. It causes an
okay-not-great error (referring to void as an "opaque type").

Change-Id: Id20a9d3512d29aecea81d46877dce708b7b2f973
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/472450
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
2021-11-17 15:21:41 +00:00
John Stiles
b2271e6016 Disallow variables of type 'void'.
Change-Id: I209119e6c74ca54dd6021b6dec4775fc7b66adeb
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/472448
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
2021-11-17 14:54:48 +00:00
John Stiles
fe3cfc1d4f Add test for variables of type void.
We should, of course, detect this and report an error.

Change-Id: I42b3be6e714a1f367d3251842506a384f2afe019
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/472447
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
2021-11-17 14:51:52 +00:00
John Stiles
33fb008669 Add parser support for u suffix on literals.
Because SkSL is much more permissive than GLSL about literal types, we
don't actually need to treat values any differently when the `u` suffix
is added. That is, `uint x = 4000000000;` already worked fine. When we
encounter the `u`, we just ignore it. This also means that a literal
like `-100u` would be accepted without complaint (although you'd get a
range error if you tried `uint x = -100u;`).

The value-add here is that it removes a speed bump when porting GLSL
code to SkSL. The Filament example shader used the `u` suffix anywhere
that bitwise ops were present; finding and removing all of them was a
chore.

Also of note: the `u` suffix was only added to GLSL in ES3, but we
"support" it everywhere. (We could go out of our way to detect it in
ES2 and flag an error, but that benefits no one.)

Change-Id: I4bf643612c8cf17710e9bad50a0c16f5936bbe88
Bug: skia:12634
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/471756
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
2021-11-16 13:50:14 +00:00
John Stiles
e10839f998 Add basic unit test for octal parsing support.
This is required by the ES2 standard: http://screen/Qysv4fPW5r5LA9e
This actually already worked fine because `strtoull` natively recognizes
octal values without any work on our part. However, we lacked a test.

Change-Id: I3033de899918abe99c63a9b7b79bd4c3374ee315
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/471716
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
2021-11-16 13:49:41 +00:00
John Stiles
14a487fd54 Replace getConstantSubexpression with getConstantValue.
The only type of expressions that getConstantSubexpression could ever
return are Literal and nullptr. getConstantValue now returns an
optional<double>; nullopt indicates a non-constant value in the slot.
This simplifies most use cases, and allows us to get rid of some extra
"zero" and "one" Literal objects in some of our Constructor classes.

This change fixes a recent fuzzer issue. The fuzzer had discovered that
calling `getConstantSubexpression` on a ConstructorCompoundCast that
contained a compile-time-constant value would return literals of the
wrong type (the cast was not applied). By nesting repeated matrix casts,
this type confusion could be turned into an assertion.

Change-Id: Icee69219e6db2822ffdfab4e5ccdaff54584a4b6
Bug: oss-fuzz:41000
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/471376
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2021-11-15 14:46:21 +00:00
John Stiles
7ab86bd3e3 Add variable slot information to SkVMDebugInfo.
This assigns a human-readable name to a debug slot. The slot map is
emitted into skslc output files, and will be used in the future to
display human-readable names in the debugger.

Change-Id: I288358de305239005faa5814bd1d77a38b5e05b0
Bug: skia:12614
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/470400
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>
2021-11-15 14:24:08 +00:00
John Stiles
6460541ee4 Reland "Fix Metal codegen error with structs containing compound types."
This reverts commit 3aaed99930.

Reason for revert: removing changes to PrecisionQualifiers

Original change's description:
> Revert "Fix Metal codegen error with structs containing compound types."
>
> This reverts commit 2a6c41571b.
>
> Reason for revert: causing Mali G7x failures on tree
>
> Original change's description:
> > Fix Metal codegen error with structs containing compound types.
> >
> > While working on an unrelated test, I accidentally triggered a bug in
> > Metal code generation. Our struct-equality helper functions did not
> > properly handle vector fields. Wrapping each comparison in `all(...)`
> > fixes the problem. (all() on a scalar is allowed and does nothing.)
> >
> > Our struct comparison tests now include a vector and a matrix.
> >
> > Change-Id: I59061ae9c3c3ab2c2dbdcb5257bc23e2257152af
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/470399
> > Commit-Queue: John Stiles <johnstiles@google.com>
> > Auto-Submit: John Stiles <johnstiles@google.com>
> > Reviewed-by: Brian Osman <brianosman@google.com>
>
> TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com,skcq-be@skia-corp.google.com.iam.gserviceaccount.com
>
> Change-Id: Ieb5d5a1839978fb82525863488e9d54fdf44adbd
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/471097
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Reviewed-by: Chris Dalton <csmartdalton@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>

Change-Id: I8ee90df3de075cf82c0fcf3b4787577b09bb1a70
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/471156
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2021-11-15 13:29:06 +00:00
John Stiles
3aaed99930 Revert "Fix Metal codegen error with structs containing compound types."
This reverts commit 2a6c41571b.

Reason for revert: causing Mali G7x failures on tree

Original change's description:
> Fix Metal codegen error with structs containing compound types.
>
> While working on an unrelated test, I accidentally triggered a bug in
> Metal code generation. Our struct-equality helper functions did not
> properly handle vector fields. Wrapping each comparison in `all(...)`
> fixes the problem. (all() on a scalar is allowed and does nothing.)
>
> Our struct comparison tests now include a vector and a matrix.
>
> Change-Id: I59061ae9c3c3ab2c2dbdcb5257bc23e2257152af
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/470399
> Commit-Queue: John Stiles <johnstiles@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>

TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com,skcq-be@skia-corp.google.com.iam.gserviceaccount.com

Change-Id: Ieb5d5a1839978fb82525863488e9d54fdf44adbd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/471097
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Chris Dalton <csmartdalton@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2021-11-12 23:09:34 +00:00
John Stiles
2a6c41571b Fix Metal codegen error with structs containing compound types.
While working on an unrelated test, I accidentally triggered a bug in
Metal code generation. Our struct-equality helper functions did not
properly handle vector fields. Wrapping each comparison in `all(...)`
fixes the problem. (all() on a scalar is allowed and does nothing.)

Our struct comparison tests now include a vector and a matrix.

Change-Id: I59061ae9c3c3ab2c2dbdcb5257bc23e2257152af
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/470399
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
2021-11-12 15:54:41 +00:00
John Stiles
1991780081 Update LoopFloat/LoopInt tests to reduce hoisting.
Previously, none of our `runtime` tests relied on the input coordinate
in any way, so all of the logic was hoisted above the main loop in every
test. This CL adds an artificial reliance on the input coordinate so
that we have at least some SkVM tests with real code in the main loop.
This lets us see debug trace instructions interleaved with real code.

The input coordinate is clamped against a known uniform value
(`colorGreen` always contains 0101) so that the final test output
remains consistent in practice.

Additionally, I noticed that this test was only enabled in ES3, but
it doesn't seem to have anything ES3-specific in it, so it's now
enabled across the board.

Change-Id: Ie82f40b1060edb6071e300040ac59fb7d27094b0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/470397
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
2021-11-12 13:45:59 +00:00
John Stiles
05c41162fb Allow inlining of functions with unassigned out-params.
Our inliner would ignore any functions with `inout` parameters, because
inlining them properly was more complex than just leaving the function
call. However, real-world code can sometimes contain helper functions
that have `inout` params that are never used at all (e.g. an uber-shader
with some features turned off).

We now read the ProgramUsage and check to see whether or not the
`inout`-qualified parameter is actually modified. If it's never changed,
the function now remains a candidate for inlining.

Change-Id: I92e494f94cc070801cb9aa28bd13faa689b806b6
Bug: skia:12636
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/470299
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
2021-11-11 19:33:06 +00:00