This can eliminate const variables which have been completely folded
away, unnecessary synthetic variables created during codegen/inlining,
or code that simply didn't need to exist at all.
Change-Id: I37a65e455e6527a6a6c2f4dde918f48d84dc2638
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/383496
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
A global variable should be considered "dead" if it's never written and
never read. The previous code checked if it was never written OR never
read, which is not the same.
This would generate GLSL/Metal that didn't compile. In SPIR-V, it would
SkASSERT, then crash, during codegen. The fuzzer was able to detect the
SPIR-V issue, but it was wrong in all three cases.
Change-Id: Id59a2499eb5baa3839b93826bfbc24191bfd490b
Bug: oss-fuzz:32005
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/385280
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 e7a8f85e4f.
Reason for revert: must revert dependent CL
Original change's description:
> Only include header once in combined MSL shader.
>
> Bug: skia:11389
> Change-Id: I3e24dcaa2cfeddc7efd7985f9f42a59bfc8175f2
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/385137
> Reviewed-by: John Stiles <johnstiles@google.com>
> Commit-Queue: Jim Van Verth <jvanverth@google.com>
TBR=jvanverth@google.com,johnstiles@google.com
Change-Id: I7a886b6c57a666e54e65365e41dcb57bd9ab4ba6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:11389
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/385237
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
'in' variables without locations aren't allowed. Use uniforms instead.
Bug: skia:11738
Change-Id: Ic066106deb7409cff154b4be7cfb3e03a7025c7d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/385000
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This reverts commit 92748af1a5.
Reason for revert: SkSLCommaSideEffects_GPU crashing on Android
Original change's description:
> Inline functions of the form 'return (expr)' only.
>
> This drastically reduces the number of functions which we allow to be
> inlined. If this change does not hurt our performance, it will allow us
> to trivially remove hundreds of LOC. All current data leads us to
> believe that it may affect the Mali 400 but is highly unlikely to change
> results on any other device in the tree.
>
> More info: http://go/optimization-in-sksl-inliner
>
> Change-Id: Ia6b706742ce5407453e0e697b6c1f9201084c0e8
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/384858
> Auto-Submit: John Stiles <johnstiles@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I6a670dacaa58fe3386ff50375ac6d1cac4fd7f2c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/385161
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Bug: skia:11389
Change-Id: I3e24dcaa2cfeddc7efd7985f9f42a59bfc8175f2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/385137
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
This reverts commit 9e476b798f.
Reason for revert: Angry Vulkan bots
Original change's description:
> Refactored SPIR-V RelaxedPrecision handling
>
> The RelaxedPrecision decoration is now handled by nextId(), to make it
> easier to see all spots where a RelaxedPrecision decoration might be
> necessary. The goal of this initial refactor is not to actually fix the
> issues with RelaxedPrecision decorations, but rather to lay the
> groundwork for doing so in followup CLs.
>
> The initial intent of this change was to not affect the SPIR-V at all,
> saving modifications for followups, but there ended up being three kinds
> of changes to the output:
>
> 1. Doing things at nextId() time rather than later means some
> decorations move to an earlier spot in the output. This results in
> diffs, but should not cause any behavioral changes.
> 2. We were incorrectly tagging bools as RelaxedPrecision in some
> situations. By funneling things through fewer code paths, the refactor
> would have caused this to happen in even more situations, and the code
> responsible for the bug was being rewritten in this CL anyway, so it
> seemed worth just fixing the issue as part of this change.
> 3. Funneling things through fewer code paths ended up adding
> (correct) RelaxedPrecision modifiers to binary operations that had
> previously been missing them. It seemed better to just let it happen
> than to try to maintain bug-for-bug compatibility with the previous
> approach.
>
> Change-Id: Ia9654d6b5754e2c797e02226660cb618c9189b36
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/384318
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I9ada728e5fd5798bc1179640560c2e6045b7efd1
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/385158
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
The RelaxedPrecision decoration is now handled by nextId(), to make it
easier to see all spots where a RelaxedPrecision decoration might be
necessary. The goal of this initial refactor is not to actually fix the
issues with RelaxedPrecision decorations, but rather to lay the
groundwork for doing so in followup CLs.
The initial intent of this change was to not affect the SPIR-V at all,
saving modifications for followups, but there ended up being three kinds
of changes to the output:
1. Doing things at nextId() time rather than later means some
decorations move to an earlier spot in the output. This results in
diffs, but should not cause any behavioral changes.
2. We were incorrectly tagging bools as RelaxedPrecision in some
situations. By funneling things through fewer code paths, the refactor
would have caused this to happen in even more situations, and the code
responsible for the bug was being rewritten in this CL anyway, so it
seemed worth just fixing the issue as part of this change.
3. Funneling things through fewer code paths ended up adding
(correct) RelaxedPrecision modifiers to binary operations that had
previously been missing them. It seemed better to just let it happen
than to try to maintain bug-for-bug compatibility with the previous
approach.
Change-Id: Ia9654d6b5754e2c797e02226660cb618c9189b36
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/384318
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This drastically reduces the number of functions which we allow to be
inlined. If this change does not hurt our performance, it will allow us
to trivially remove hundreds of LOC. All current data leads us to
believe that it may affect the Mali 400 but is highly unlikely to change
results on any other device in the tree.
More info: http://go/optimization-in-sksl-inliner
Change-Id: Ia6b706742ce5407453e0e697b6c1f9201084c0e8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/384858
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This test fails on SPIR-V when the inliner is turned off:
error: SPIR-V validation error: Pointer operand 250[%250] must be a memory object declaration
%252 = OpFunctionCall %void %out_half %250
Change-Id: Ibfa9cef371af2eea766a4218ec8a581289ee100e
Bug: skia:11748
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/384999
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Prevents us from accepting code that can't be correctly transformed to
GLSL, like:
uniform float x;
float y = x;
(Previously, writing code like that in a runtime effect would
effectively produce the exact same code all the way through to GLSL, and
the driver would fail to compile it).
Bug: skia:11336
Change-Id: Iaa797587c4a4a7289ed59ce2736cf0bf0fc5bca3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/384698
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
A side effect of this change is that I've tried to pass GrVkAttachments
around GrVkGpu instead of GrVkTextures where I could to start the
transition within the backend code.
Bug: skia:10727
Change-Id: Ibc9553cdbd7f6ae845c56aad3f25f58e4c478e46
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379577
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
Bug: skia:11716
Change-Id: Ic09071544b5b5216b01fbc9b478b6269dd96202f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/382280
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This only affects fragmentProcessors (children) - and the backend SkSL
we're emitting should not contain those. We've just been silently
ignoring those declarations when converting to GLSL, MSL, etc.
Change-Id: I241f2f4fe4614b49ebccc9c2976fd408e94656d0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/384316
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This reverts commit a04692f69e.
Reason for revert: Angry Vulkan bots.
Original change's description:
> Fixed a number of spots where we should have been using RelaxedPrecision
>
> Our SPIR-V output was missing many RelaxedPrecision decorations, which
> was presumably impacting performance.
>
> Change-Id: Iee32d4a42f37af167fe0e45f3db94c2142129695
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/384178
> Reviewed-by: Brian Osman <brianosman@google.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
TBR=egdaniel@google.com,brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: If4fe945cb363c9b61b5a4abfde649a437689d2eb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/384217
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
I ran into an issue in an upcoming CL which generated a particularly
ugly switch statement:
switch (x) {
default:
discard;}
So I cleaned this up, and while resolving this issue, managed to improve
a bunch of existing codegen as well. The formatting change has been
split out to a separate CL since it impacts so many golden outputs.
Change-Id: I7a6be29903c47560dcc7f6acd3ef15fd0c5c3c50
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/384179
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Our SPIR-V output was missing many RelaxedPrecision decorations, which
was presumably impacting performance.
Change-Id: Iee32d4a42f37af167fe0e45f3db94c2142129695
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/384178
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
All that's left is a clip atlas renderer.
Bug: chromium:1158093
Change-Id: I8b509904a752a202ff1321e5302c41a3f57a5edb
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/383741
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Chris Dalton <csmartdalton@google.com>
Bug: skia:11738
Change-Id: I1dd5e99830f70d72c292379a45c4e39a55588858
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/383706
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Kevin Lubick <kjlubick@google.com>
Auto-Submit: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Kevin Lubick <kjlubick@google.com>
We no longer derive a performance benefit from this pass in practice,
and it is a very expensive compilation step. It is also prone to fuzz-
related errors.
Doc: http://go/optimization-in-sksl
Change-Id: Ief08ffac659a8fe7fe92c92b9a5da14c9f713bc2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/381261
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Previously even if we release/abanoned a resource we would still return
GrBackendSurfaces from these queries even if the actual backend api
objects are no longer valid.
This hopefully fixes the attached chrome bug, but can't know for sure.
Bug: chromium:1186623
Change-Id: Ib1c03699c2c7d81f6d305428dfbf39d647d28373
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/382918
Commit-Queue: Greg Daniel <egdaniel@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
This declutters the atlas generation code so there are fewer variables.
Next we will delete all the lower level rendering code and render the
atlas with stencilPath/stencilRect instead.
Bug: chromium:1158093
Change-Id: I36cff285d0f7de6f8ece4b027e62ae84aa01adc8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/380656
Commit-Queue: Chris Dalton <csmartdalton@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Abandoning would cause us to unref all the command buffer usages, but
then in notifyARefIsZero we would delete the abandoned resources even
though we still had a real ref. Then when that ref went away we would
crash.
Change-Id: I05dc2ba9a67c35c36a36704f4b81d6eef4e860e6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/382916
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
This is useful because it allows the variables to be declared as `const`
when they are trivial values like `half4(1)`. This enables the constant
folder to simplify or eliminate them. In most cases, this is only a
small benefit, as you'd expect a competent GPU driver to do the same.
However, Mali-400 can benefit significantly from optimizing away the
multiplication against a constant half4(1) coverage in Porter-Duff.
Mali-400 performance is back to normal: http://screen/3cDxdaGkYE8oBcS
Change-Id: I21fd23f91f747079cd05b082f7b3444aeabafb93
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/382476
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
As you might expect, a function tagged with `noinline` will never be
considered as a candidate for inlining.
Change-Id: Ia098f8974e6de251d78bb2a76cd71db8a86bc19c
Bug: skia:11362
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/382337
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Currently, only one of three uses (local variables) does this correctly.
Bug: skia:11716
Change-Id: Iad11e8e5998fcc7caee4d438e0558c5d4e2b1821
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/382277
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
With skia-review.googlesource.com/c/skia/+/363782, both cpu and gpu
backends would gracefully fail readPixel requests where dst row bytes
wasn't a multiple of dst bpp. It also updated the cpu backend's
writePixels behavior to gracefully reject writePixels requests where
the src row bytes wasn't a multiple of src bpp.
GPU writePixels would not detect this and later trigger an assert
in debug builds in GrConvertPixels (caught by the linked fuzzer bug).
This adds tests to mirror the read pixels bad-row-bytes tests and
updates GrSurfaceContext::writePixels to check src row bytes vs. bpp.
I confirmed it fixes the fuzzer crash.
Bug: chromium:1185266
Change-Id: I7cd8406c65a9ba35a55d695b2f65410a1edd2a19
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/382276
Auto-Submit: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
Change-Id: Ic2d1240ab785101365b0fd934562505fb5a3e599
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/381816
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
It turned out that everywhere we were using or testing DSL code either
directly or indirectly imported big chunks of the SkSL library. These
imports turned out to be necessary; code written using just DSL.h would
fail with various template instantiation errors.
Change-Id: Iae72d15b0d6ef14614ac1a4ff08c36bc1876cd4d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/381638
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
The crash is because method GrGpuBuffer::onRelease() is called on wrong
thread. SkMessageBus::Post(const Message& m) takes a const ref of the
message, so there is a little possibility the GPU thread received and
handled the message before SkMessageBus::Post() is returned. In that
case, the caller of SkMessageBus::Post() (AsyncReadResult) still holds
the last ref of the GrGpuBuffer, and then GrGpuBuffer() will just be
released on the wrong thread.
Bug: chromium:1185489
Change-Id: I28665dbb1db7925d59ec574e9e26385e845ff4df
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/380696
Commit-Queue: Peng Huang <penghuang@chromium.org>
Commit-Queue: Brian Salomon <bsalomon@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
Auto-Submit: Peng Huang <penghuang@chromium.org>
These were accidentally omitted from the supported operator list.
Change-Id: Idecd17adb8b3f5043e36328c65ca12be33e990f4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/381637
Auto-Submit: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Expressions like `value == value` or `color.a != color.a` can be
replaced by `true` or `false` on sight. The GLSL spec makes it clear
that checking for NaN is optional:
4.7.1 Range and Precision
"... NaNs are not required to be generated. Support for signaling NaNs
is not required and exceptions are never raised. Operations and built-in
functions that operate on a NaN are not required to return a NaN as the
result."
Change-Id: I2ad9f2dc505b638ea2904bef41b7a79a2b329551
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/381262
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This CL will be used to test for potential performance regressions (or
improvements) that we might cause by disabling this optimization pass.
It will be reverted in ~1 day.
Change-Id: I26b7687c341eb6d81231406381c39869cfccf6d6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/381259
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
There's really only one failure at this point, and it's a failed
instantiation. The original intention of the allocation failure
system was to only drop the ops that referred to the uninstantiated
proxies, but somewhere along the way that was lost and
we started dropping arbitrarily large chunks of ops. Lets just
bail and not crash instead.
Bug: skia:10877
Change-Id: I675358e8a1fbd2d75ea29b72ccfc50c7df90343e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/371337
Commit-Queue: Adlai Holler <adlai@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
Change-Id: I7a7874e58bf53978afce8a41b26092406b6490ed
Bug: skia:11342
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/380360
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This reverts commit 9ef3f2e3da.
Reason for revert: relanding with fix
Original change's description:
> Revert "Have GrVkRenderTarget only use GrVkAttachments and not derive from GrVkImage."
>
> This reverts commit 3dc6c190da.
>
> Reason for revert: hitting assert about RT having input attachment on mali bots
>
> Original change's description:
> > Have GrVkRenderTarget only use GrVkAttachments and not derive from GrVkImage.
> >
> > This change moves the color and resolve attachments used in a
> > GrVkRenderTarget to be a GrVkAttachment. These along with the msaa
> > attachment now mean that GrVkRenderTarget no longer needs to derive from
> > a GrVkImage.
> >
> > There are a couple ugly things in this CL since GrVkTexture still is a
> > GrVkImage since we can't share attachments between GrVkRT and GrVkTex.
> > But when that gets updated in the follow on CL things will look much nicer.
> >
> > Bug: skia:10727
> > Change-Id: I2f12674d7517c6d6dea389e2d1fb7296028bcc85
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379576
> > Reviewed-by: Brian Salomon <bsalomon@google.com>
> > Commit-Queue: Greg Daniel <egdaniel@google.com>
>
> TBR=egdaniel@google.com,jvanverth@google.com,bsalomon@google.com
>
> Change-Id: Ic46f3947ed9f7b2ca26e8418d643e7f89b6108d2
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: skia:10727
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/380459
> Reviewed-by: Greg Daniel <egdaniel@google.com>
> Commit-Queue: Greg Daniel <egdaniel@google.com>
# Not skipping CQ checks because this is a reland.
Bug: skia:10727
Change-Id: I7a995ee9ad35bdac34cfcfd6b0d963c3e0bb90b8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/380460
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
Expressions like `x * 1`, `x *= 1`, `x + 0`, `x * 0`, or `0 / x` don't
actually do anything, and can be simplified to just `x` or `0`. (The
zero case must also check that `x` doesn't have side effects, because
`0 * myFunction()` still needs to call `myFunction`.)
`0 - x` is also detected and rewritten as `-x`.
`0 / 0` is left as-is.
This logic works for scalars and vectors; matrices are left as-is.
A similar optimization also occurs in the constant-propagation pass, so
we see almost no diffs in the tests. If control-flow analysis is turned
off, we do see some improvements. (I didn't reuse the existing code at
all, since it was designed around rewriting the CFG tree, but the
concept was identical.)
Change-Id: Ia99cd81f1d4cd3dafaa43ccac6a2261e3257a185
Bug: skia:11343
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/380356
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit 3dc6c190da.
Reason for revert: hitting assert about RT having input attachment on mali bots
Original change's description:
> Have GrVkRenderTarget only use GrVkAttachments and not derive from GrVkImage.
>
> This change moves the color and resolve attachments used in a
> GrVkRenderTarget to be a GrVkAttachment. These along with the msaa
> attachment now mean that GrVkRenderTarget no longer needs to derive from
> a GrVkImage.
>
> There are a couple ugly things in this CL since GrVkTexture still is a
> GrVkImage since we can't share attachments between GrVkRT and GrVkTex.
> But when that gets updated in the follow on CL things will look much nicer.
>
> Bug: skia:10727
> Change-Id: I2f12674d7517c6d6dea389e2d1fb7296028bcc85
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379576
> Reviewed-by: Brian Salomon <bsalomon@google.com>
> Commit-Queue: Greg Daniel <egdaniel@google.com>
TBR=egdaniel@google.com,jvanverth@google.com,bsalomon@google.com
Change-Id: Ic46f3947ed9f7b2ca26e8418d643e7f89b6108d2
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:10727
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/380459
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
Declare
This solves several issues caused by the lack of ordering guarantees in
C++; it was possible for the SkSL backend to look for the value of a
variable before its Declare() call gets processed. Moving the initial
value out of Declare should fix this whole class of problems.
Change-Id: I428fe230f1c312a0128c1f00c2a36cb95f4590a6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/380358
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This enables the ternary to be optimized away in code like:
const bool SHINY = true;
color = SHINY ? add_shine(x) : x; // to --> `color = add_shine(x);`
Without constant propagation.
Also, I added a unit test for ternary expression simplification; I
wasn't able to find an existing one.
When the optimization flag is disabled, this CL actually removes the
optimization of `true ? x : y` --> `x` entirely; previously, this
substitution would be made regardless of optimization settings.
Change-Id: I93a8b9d4027902d35f8a19cfd6417170b209d056
Bug: skia:11343
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379297
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This change moves the color and resolve attachments used in a
GrVkRenderTarget to be a GrVkAttachment. These along with the msaa
attachment now mean that GrVkRenderTarget no longer needs to derive from
a GrVkImage.
There are a couple ugly things in this CL since GrVkTexture still is a
GrVkImage since we can't share attachments between GrVkRT and GrVkTex.
But when that gets updated in the follow on CL things will look much nicer.
Bug: skia:10727
Change-Id: I2f12674d7517c6d6dea389e2d1fb7296028bcc85
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379576
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
Swizzle optimizations now occur at IR generation time. These
optimizations are redundant with the control-flow optimization phase so
they are mostly not visible in our test output, but they do affect DSL
test results. Interestingly, they do improve our test output slightly
as well, for various reasons (e.g. we do not fully optimize lvalues in
the control-flow pass).
Change-Id: I6ebe6d71a5c22d9823b5fa500e43078915cbfb45
Bug: skia:11343
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372257
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Makes SkImage_Gpu backed by two proxies, an original and a copy. The
image uses the original until a new render task is bound to it at which
point further uses of the image will use the copy. If the image is ever
used off a GrDirectContext we fall over to the copy. If the copy is
never used and never can be used by the next flush then the render
task that populates it is marked "skipped" and we don't perform the
copy.
Bug: skia:11208
Change-Id: Id255f4a733acc608c8a53c1a5633207aeafc404b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/366282
Commit-Queue: Brian Salomon <bsalomon@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
This is a reland of 40a9061203
Original change's description:
> rename GrSDFTOptions to GrSDFTControl
>
> Change-Id: Ie03fce7a99a9f71b18d54e3cd35e7675fb7f8912
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379616
> Commit-Queue: Herb Derby <herb@google.com>
> Reviewed-by: Robert Phillips <robertphillips@google.com>
Change-Id: Iba0816159a8ef99448a0040c487e56700b96be5d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379845
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Herb Derby <herb@google.com>
This performs the same simplifications as the control-flow phase, but at
IR generation time. There's no visible difference in the tests (besides
DSL) because these aren't new optimizations; they're just happening
at a different phase of compilation.
Change-Id: I26d241167b0e690b23f8f4370339714783c8d6fd
Bug: skia:11343
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/371482
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 check now runs at function finalization time, before constant
propagation has occurred; this affected the "DeadIfStatement" test.
Our detection isn't smart enough to realize that a loop will run zero
times, so it treats `for` and `while` loops as always running at least
once. This isn't strictly correct, but it actually mirrors how the CFG
implementation works anyway. The only downside is that we would not flag
code like `for (i=0; i<0; ++i) { return x; }` as an error.
Change-Id: I5e43a6ee3a3993045559f0fb0646d36112543a94
Bug: skia:11377
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379056
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
In addition to the unsurprising changes to eliminate references to
src/, we also had to tighten up some C++17-isms as they are not
permitted in public headers.
Change-Id: Ie5005a33d7a135e69fb66beca5e7a5f960dbd453
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/378496
Reviewed-by: Brian Osman <brianosman@google.com>
PictureShader = picture + tiling + depth/colorspace + filtering [+ scale]
Today we cache the imageshader that is used to rendering. However, the
key for that cache is the pictureshader's ID itself... which means if
we have several, all using the same picture (but maybe diff tiling) we
would create dup cache entries.
Idea:
1. only cache the image (rastered picture), not an imageShader
2. key the cache on the picture's ID, not the shader's
Several implications of this:
1. Should get more cache reuse, since we don't care about the
shader's ID (which is just wrapping a picture+tiling, etc.)
2. We also eliminate the indirection of creating a PictureImage. Instead
we're creating real (pixel) images, and caching those. This removes one
extra layer of "cache".
Idea: when we cache something for pict
Change-Id: I51cf4e9bff3c91ce1872876597d3d565039d8c7a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/377844
Commit-Queue: Mike Reed <reed@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
This should be legal, and we support this, but some versions of Android
do not: http://screen/3bkQewHF3xUMn5v There's no point in allowing
these shaders to exist; they can't compile on real-world clients, and
these vardecls are borderline meaningless (as the variables being
declared aren't reachable by any other statements).
Change-Id: Ie1351933c90caee9124eeab8983364ec030b2653
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379584
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This reverts commit 50b1b2b90d.
Reason for revert: ending experiment
Original change's description:
> Disable control-flow analysis in SkSL. (Performance experiment)
>
> This CL will be used to test for potential performance regressions (or
> improvements?) that we might incur by disabling this optimization pass.
>
> It will be reverted in ~1 day.
>
> Change-Id: I775cdb0c95df81fa25ebbd66e4ff01f64c660f68
> Bug: skia:11319
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/378456
> Commit-Queue: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: Ie385a82db237ff5651348d82b9651f8ba09375b9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:11319
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379581
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Covers some common geometry processors, texture effect, etc.
This also rearranges how fp keys are arranged in the overall key. We no
longer include the key size as part of the key - this made no sense.
Instead, we explicitly include the number of children. We also put all
data for one fp before any children, so the tree can be reconstructed
more-or-less top-down.
Finally, added an "addBool" helper that reads nicer than addBits(1)
everywhere.
Bug: skia:11372
Change-Id: I4e35257fb5923d88fe6d7522109a0b3f4c4017d4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379059
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This CL will be used to test for potential performance regressions (or
improvements?) that we might incur by disabling this optimization pass.
It will be reverted in ~1 day.
Change-Id: I775cdb0c95df81fa25ebbd66e4ff01f64c660f68
Bug: skia:11319
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/378456
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This reverts commit e4da7b672f.
Reason for revert: breaks SkSLBench perf test
Original change's description:
> Migrate if-statement simplifyStatement logic to IfStatement::Make.
>
> This performs essentially the same simplifications as before, just at
> a different phase of compilation.
>
> Change-Id: Ia88df6857d4089962505cd1281798fda74fd0b02
> Bug: skia:11343, skia:11319
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/376177
> Commit-Queue: John Stiles <johnstiles@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I0051188ffe69426904066eb60a932435efdc2af8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:11343
Bug: skia:11319
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/379062
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
All layout(key) fields include the field name meta-data, and use as few
bits as possible.
Bug: skia:11372
Change-Id: Ie12b3e0d01148457e5ea078cbf7d0a4bff35302e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/378596
Reviewed-by: Brian Salomon <bsalomon@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This performs essentially the same simplifications as before, just at
a different phase of compilation.
Change-Id: Ia88df6857d4089962505cd1281798fda74fd0b02
Bug: skia:11343, skia:11319
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/376177
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Rather than have the inliner own this responsibility, the function
finalizer now detects if a function is supposed to return a value but
never actually does. This will allow us to detect this error case even
if the inliner is disabled. The inliner should no longer encounter
functions that claim to return a value but don't, so it will now assert
if one is encountered. (The inliner still has the logic to handle this
case gracefully, just in case.)
The check is currently very simple and doesn't analyze the structure of
the function, so it won't report cases where some paths return a value
and others don't, e.g. this will pass the test:
int func() { if (something()) return 123; }
(This is good enough to resolve the inliner issue, though, as it only
occurred in functions with no value-returns at all.)
Change-Id: I21f13daffe66c8f2e72932b320ee268ba9207bfa
Bug: oss-fuzz:31469, oss-fuzz:31525, skia:11377
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/377196
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
The logic for picking what type of sub run to create is spread
over several locations. Gather that altogether in drawingType().
Have GrSDFT close over all the data needed to calculate the
drawing type. This reduces plumbing to the processGlyphRun
routine.
The next CL should rename GrSDFTOptions to GrSDFTControl.
Change-Id: I99e74c11af6d3b3d9919e54fe1e7286fcfbf1bfb
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/378036
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Herb Derby <herb@google.com>
Bug: skia:11356
Change-Id: I16322e6396dc7e7c8c50ba1d39e07311cf3bd346
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/376116
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Interestingly, this improves our codegen even with the optimizer fully
enabled, as apparently statement chains like:
`x = true; x = x; x = x;`
were getting transformed by constant-propagation into:
`x = true; x = true; x = true;`
making them no longer candidates for self-assignment elimination.
Change-Id: I6d94a809e94b01a00fd92459fcbce898b3cbbb11
Bug: skia:11343
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/377100
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
In http://review.skia.org/375776, an optimization was added to the
Inliner, causing it to skip generation of unnecessary temporary
variables. The fuzzer immediately discovered a flaw in this logic: the
"unnecessary" variable was actually used in the rare case that a
function failed to actually return a value. The inliner didn't detect
this case. Of course, this isn't a valid program either, so now we
report the error and cleanly fail.
Change-Id: I1f201cfd33f45cace3be93765a4e214e43a46e69
Bug: oss-fuzz:31469, oss-fuzz:31525
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/377101
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Surprisingly, this actually improved our error detection slightly.
The expression `- -half4(0)` can now be simplified to `half4(0)` at
IR generation time, which allows the constant-folder to detect a
constant zero (and from there, a division by constant zero).
Change-Id: I8c4f6ab522efab5bf98913f9c6a1487b7af39a99
Bug: skia:11342, skia:11343
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/376842
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
For now, just use this to prevent *any* layout qualifiers from appearing
on functions, or their parameters.
Bug: skia:11301
Change-Id: I05d8118c7121048c6ef49695a54e3714a8f8687e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/376796
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This adds Analysis::IsConstantExpression, to determine if an expression
is a constant-expression. It now expands to cover 'const' local and
global variables, because we also enforce that the initializer on those
variables is - in turn - a constant expression.
This fixes 10837 - previously you could initialize a const variable with
a non-constant expression, and we'd emit GLSL that contained that same
pattern, which would fail to compile at the driver level. That should
not be possible any longer.
Bug: skia:10679
Bug: skia:10837
Change-Id: I517820ef4da57fff45768c0b04c55aebc18d3272
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/375856
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
The expression `~123` was making a PrefixExpression of type $intLiteral.
It should be converted to type `int` when the ~ prefix is applied.
This change also changes the output from oss-fuzz:27614. Both programs
are essentially nonsense expressions with no real behavior, so this is
fine.
Change-Id: I586be149ce95136fabee72fdd3473814d54948cf
Bug: oss-fuzz:31410
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/376620
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
No point holding two copies of our data, an SkBitmap and fStorage array.
The GPU backend wants a bitmap, so make the CPU backend work from that.
Updating the flattened format removed the last use of SkPackBits.
Change-Id: I97f75ebe9324ec5eed37dfe6282e10d0fe08711f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/376599
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Adlai Holler <adlai@google.com>
This reverts commit 4903482a2a.
Change-Id: Ie56583344897fcb2c072bcac4d97d582edeadf07
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/375660
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
These variables were later being eliminated by the dead-code-elimination
pass, so you can't see them directly in the final output, but removing
them affects the name mangling off all future symbols, so it causes an
enormous ripple effect in the diff. And of course, it's a waste of time
and memory to synthesize IRNodes just to destroy them later.
If we disable control-flow analysis, we lose the dead-code-elimination
pass entirely; this change is also beneficial for emitting better code
when optimizations are turned off.
Change-Id: I882b3be4f3fd99b77d99b6abe128f26bb9252c89
Bug: skia:11319
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/375776
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
- Migrate our code to SkImage::MakePromiseTexture
- Have DDLTileHelper share one SKP and one set of promise images across all tiles.
- Disallow on-the-fly allocation of mips for promise textures.
Bug: skia:10286
Change-Id: Ie35976958454fc520f3c9d860e6285441260c9f7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/291938
Commit-Queue: Adlai Holler <adlai@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
This reverts commit d42932a1b5.
Reason for revert: Breaking CMake and SKQP builds.
Original change's description:
> Add DSL PossibleExpression & PossibleStatement
>
> These are currently unused, but in future CLs they will be used to
> capture line number information in DSL error handling.
>
> Change-Id: Ieee730e0ad8323043437972fedb5bec471c367e4
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/375069
> Reviewed-by: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I2062c9964569c9129a2145f9e45f849310129687
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/375658
Reviewed-by: Florin Malita <fmalita@google.com>
Commit-Queue: Florin Malita <fmalita@google.com>
This reverts commit 8a43a2889e.
Reason for revert: Prereq revert for https://skia-review.googlesource.com/c/skia/+/375069
Original change's description:
> SkSL DSL now reports the C++ line numbers of errors
>
> This is done a best-effort basis. Positions will only be reported in
> Clang and GCC, and may not be reported in every possible situation.
>
> Change-Id: I7efb15781546b95e667fee34e488366174bb5f36
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/374600
> Reviewed-by: John Stiles <johnstiles@google.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I173cb355533d6f99c9561177b68f56924fe8a98f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/375657
Reviewed-by: Florin Malita <fmalita@google.com>
Commit-Queue: Florin Malita <fmalita@google.com>
This is done a best-effort basis. Positions will only be reported in
Clang and GCC, and may not be reported in every possible situation.
Change-Id: I7efb15781546b95e667fee34e488366174bb5f36
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/374600
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Change-Id: Iafeb13812851271a5262730e9c0642d4469c273f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/375020
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
These are currently unused, but in future CLs they will be used to
capture line number information in DSL error handling.
Change-Id: Ieee730e0ad8323043437972fedb5bec471c367e4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/375069
Reviewed-by: John Stiles <johnstiles@google.com>
Previously these existed in each Program, but they are unchanging for
the lifetime of the Compiler. Also, their presence in the Context will
allow the SkSL::Setting class to access them outside of the IRGenerator.
Also, removed some code which allowed the IRGenerator to support null
shader-caps; this never actually happens. (The Compiler asserts that
they are non-null; the GLSL code generator uses the caps bits without a
null check, etc.)
Change-Id: Id58743a0c2ac068cb60ec35648b4f758bf28bbe3
Bug: skia:11365, skia:11319
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/375067
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This will allow users to create PDFs with the actual text embedded. This
will allow for correct search and copy operations on the generated PDF.
Since these are now public, SkTextBlobBuilderPriv is no longer needed
and is removed. For consistency, the allocRunRSXform overload is renamed
to allocRunTextRSXform.
Change-Id: I44be82d9038a433e1221d5cbfd8ed113ecb6d4fa
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/375017
Reviewed-by: Mike Reed <reed@google.com>
Commit-Queue: Ben Wagner <bungeman@google.com>
Not as many casts needed to use this.
Change-Id: I871b5f3f9658d349d08d396f2cb8ec5498c3b50b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/375061
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Herb Derby <herb@google.com>
The 'lang' was never stored or used, so this makes no current practical
difference. The original intent was to be able to specify a language so
that it could be emitted as a 'Lang' override in the 'ActualText' when
generating a PDF. However, due to the way 'ActualText' is generally used
this would be impractical. If there is ever a desire to mark up sections
of the PDF with a specific language it would be better handled in a
different way.
Change-Id: Id63596190235fc45ce17249b9b578b6f9b838b2b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/375060
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Ben Wagner <bungeman@google.com>
Now, even if a qualifier has a default value, we will know that it
appeared in the text. We can use that to check for redundant qualifiers
(as is being done here), and in the IR generator to prevent any use of
certain qualifiers, depending on context. (eg, runtime effects, wrong
shader stage, on a parameter declaration, etc.)
Bug: skia:11301
Change-Id: I2cd6ad35c2b4c4d6f87ade97e80aea84dc16ee4b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/374616
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
GrStrokeHardwareTessellator is the only user that needs to simply
check if a cusp exists at all. Making the check local speeds up the
microbench from 502us -> 392.
Bug: chromium:1172543
Change-Id: I52adf9ef0a66e1267435e6ce41f01ee1eda17d7a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/373744
Reviewed-by: Greg Daniel <egdaniel@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Chris Dalton <csmartdalton@google.com>
At IR generation time, this CL limits our optimizations to only
@switch statements. A regular switch statement will only be optimized
during the optimization phase even if the switch-value is a known
compile-time constant. This is done to avoid upsetting our reachability
analysis.
Most of this CL is moving existing logic from SkSLCompiler into
SkSLAnalysis and SkSLSwitchStatement. Although the diffs look large, the
actual changes are very small.
Change-Id: I90920f41bc386dfa7a980ae7510f6681231a5120
Bug: skia:11340, skia:11342, skia:11319
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372679
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Bug: skia:10837
Change-Id: I33da2eb1e723ed04ab62d65c21e54306dd362bed
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372677
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Rewrite as one cacheable runtime effect with uniforms paralleling
SkHighContrastConfig. This is mostly for style, in the sense of
readability but also how I'd recommend writing these effects generally:
one program body controlled by uniforms. Callers needing specialization
can write their own effect (or use our TBD uniform->constant facility).
Test isAlphaUnchanged(), and make it so by not saturating c.a.
MaliT880 was producing seriously bad results until I switched to mix().
Change-Id: Ia4cfef25fe26643f6832a5fa0843b28dafc5b284
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372295
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Change-Id: I10d561eec456a7917681d7bdf0b1bd2f5ee5ad5f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/374217
Auto-Submit: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This is the only place we provide a proxy from a SkImage other than
asView() and is only used in one place in the library. Add a utility
function to replace uses in tests based on asView().
Bug: skia:11208
Change-Id: Ica6eae6e5f153f208523331566acc0360925a20e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/373617
Commit-Queue: Brian Salomon <bsalomon@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
Rename factory function from createGLSLInstance() to makeProgramImpl()
Bug: b/180759848
Bug: skia:11358
Change-Id: I095bdf1f26db5a8192fa8ab59000db4a1d561d96
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/373738
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
The test inputs were removed at http://review.skia.org/360778
Change-Id: Ib2918f3f984cd80463bacb822ef510ee9feb1e77
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/373916
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This splits switch() construction into two stages.
- One version of Make takes an array of case-values and case-statement
lists, and is responsible for reporting errors if the case-values are
not unique or are improperly typed. This is what the IR generator or DSL
will start with on its first encounter with the switch statement.
- The other version of Make takes an array of already-processed
SwitchCases and can assume the invariant that they're all correctly-
typed with unique values. This is what we will have when a statement
is inlined or otherwise cloned. (We still assert this invariant, for
correctness' sake, but in release mode we assume it.)
This CL doesn't perform any optimizations at Make time yet; it does work
equivalent to how `switch` works in the IR generator today. It does
improve duplicate case-label checking slightly; duplicate case labels
are now reported, and duplicate `default:` labels are detected.
(Multiple `default` labels won't pass the parser, but they can be
constructed in DSL.)
Change-Id: I537ce2c8236152d58641fb1793619d66a62c01a8
Bug: skia:11342
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372616
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
These were unused - we always enable the advanced blend equation
extension using "blend_support_all_equations" (if enabling the
extension is required at all).
Change-Id: I95fd6483ec54dfaf983290de95629fe0e86c22e8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/373877
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Chris Dalton <csmartdalton@google.com>
Constant propagation might be going away, but static-switches are likely
here to stay. Avoid conflating the two in this test.
Change-Id: If4b6c99c85f124d3bbc20da858693f09f5e4fd59
Bug: skia:11319
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/374117
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
The optimizer now properly recognizes all types of exits from a switch
statement. Break, continue and return are all potential exits and need
to be considered when determining the exit path from the switch.
Previously, dead code elimination was hiding the effects of this bug
from us, but it meant that an optimized switch had the potential to
generate lots of worthless IR nodes which then needed to be detected and
eliminated by the CFG. In particular, this affected the enum form of
blend, causing a catastrophic amount of extra work to be done.
Change-Id: If857e38cadfc016884624ea4db25a273ad3dce5b
Bug: skia:11352
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372958
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This allows things to align better between the IR and DSL sides
and gives us equivalent error handling on both sides.
Change-Id: I6d5569e29df51a4d1a6cb0ad1e6611d419dfe30c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/373737
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I672345116e3b5538c0f7e8c5f2f74aa56bb81e6d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372676
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
The intent of this class was to abstract the internal linked list used
by GrStrokeTessellateOp, but it seems to just make things more
complicated. We have a need now to iterate the list with more freedom
than is offered by GrSTArenaList, so it seems best to just use a plain
C-style linked list instead.
Bug: chromium:1172543
Change-Id: Ia76be83c523bd3c285200099a529ccd3818490b6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372656
Commit-Queue: Chris Dalton <csmartdalton@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
When we detect a static switch, the optimizer finds the matching switch-
case and eliminates all the other switch-cases. It handles case
fall-through by scanning forward and looking for an unconditional break.
However, the inliner has an interesting quirk--it can replace `return`
statements inside of a switch with `continue` statements, since the body
of the inlined function has been wrapped with a for-loop to allow for
early exits. The optimizer does not recognize these continue statements
as exits from the switch (although they certainly qualify), so it
treats continues as fallen-through and keeps emitting switch-cases.
The dead-code elimination pass was actually doing us a favor here and
eliminating the excess code later. A flag was added to disable DCE in
order to reveal the problem in a test.
Change-Id: I8ff19fde5e32d0ab73d7c5411da40cb953a446f5
Bug: skia:11352
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372956
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This reverts commit ba55be671d.
Reason for revert: G3 roll
(08:59:24) ERROR: third_party/skia/HEAD/BUILD:926:10: Compiling third_party/skia/HEAD/tests/TypefaceMacTest.cpp failed: (Exit 1) driver_is_not_gcc failed: error executing command third_party/crosstool/v18/stable/toolchain/bin/driver_is_not_gcc '-frandom-seed=blaze-out/k8-fastbuild/bin/third_party/skia/HEAD/_objs/dm/TypefaceMacTest.pic.o' -DSK_USE_FREETYPE_EMBOLDEN ... (remaining 383 argument(s) skipped). [forge_remote_host=ixog19]
third_party/skia/HEAD/tests/TypefaceMacTest.cpp:31:45: error: unknown type name 'CTFontRef'
auto makeSystemFont = [](float size) -> CTFontRef {
^
third_party/skia/HEAD/tests/TypefaceMacTest.cpp:33:46: error: use of undeclared identifier 'kCTFontUIFontSystem'
return CTFontCreateUIFontForLanguage(kCTFontUIFontSystem, size, nullptr);
^
2 errors generated.
Original change's description:
> Reland "Test mac system font variations."
>
> This reverts commit 4c0b9b90d6.
>
> Reason for revert: Work around broken -Wrange-loop-analysis
>
> Original change's description:
> > Revert "Test mac system font variations."
> >
> > This reverts commit a612dc77d7.
> >
> > Reason for revert: Breaking iOS builds.
> >
> > Original change's description:
> > > Test mac system font variations.
> > >
> > > On macOS system fonts are special and sometimes have different behavior
> > > from fonts generated from data. Add a test which exercises several
> > > expectations about changing the variation on the system ui font.
> > >
> > > Bug: skia:10968
> > > Change-Id: Ia10dfbf7f4f0ff099f9bfebf95481c95c7d3715f
> > > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372218
> > > Commit-Queue: Ben Wagner <bungeman@google.com>
> > > Reviewed-by: Herb Derby <herb@google.com>
> >
> > TBR=bungeman@google.com,herb@google.com,drott@google.com
> >
> > Change-Id: Iccc05f25d827ab85c507b5f3bde936561349e2b8
> > No-Presubmit: true
> > No-Tree-Checks: true
> > No-Try: true
> > Bug: skia:10968
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372678
> > Reviewed-by: Jim Van Verth <jvanverth@google.com>
> > Commit-Queue: Jim Van Verth <jvanverth@google.com>
>
> # Not skipping CQ checks because this is a reland.
>
> Bug: skia:10968
> Change-Id: Ifddc6c5ada335d97f7796df7f6ea10577f6bc252
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372776
> Commit-Queue: Ben Wagner <bungeman@google.com>
> Reviewed-by: Ben Wagner <bungeman@google.com>
# Not skipping CQ checks because original CL landed > 1 day ago.
Bug: skia:10968
Change-Id: Ia5ff4ff827e3f79ff17b4d99458ffb45b7c36c58
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/373277
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This reverts commit 4c0b9b90d6.
Reason for revert: Work around broken -Wrange-loop-analysis
Original change's description:
> Revert "Test mac system font variations."
>
> This reverts commit a612dc77d7.
>
> Reason for revert: Breaking iOS builds.
>
> Original change's description:
> > Test mac system font variations.
> >
> > On macOS system fonts are special and sometimes have different behavior
> > from fonts generated from data. Add a test which exercises several
> > expectations about changing the variation on the system ui font.
> >
> > Bug: skia:10968
> > Change-Id: Ia10dfbf7f4f0ff099f9bfebf95481c95c7d3715f
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372218
> > Commit-Queue: Ben Wagner <bungeman@google.com>
> > Reviewed-by: Herb Derby <herb@google.com>
>
> TBR=bungeman@google.com,herb@google.com,drott@google.com
>
> Change-Id: Iccc05f25d827ab85c507b5f3bde936561349e2b8
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: skia:10968
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372678
> Reviewed-by: Jim Van Verth <jvanverth@google.com>
> Commit-Queue: Jim Van Verth <jvanverth@google.com>
# Not skipping CQ checks because this is a reland.
Bug: skia:10968
Change-Id: Ifddc6c5ada335d97f7796df7f6ea10577f6bc252
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372776
Commit-Queue: Ben Wagner <bungeman@google.com>
Reviewed-by: Ben Wagner <bungeman@google.com>
This reverts commit a612dc77d7.
Reason for revert: Breaking iOS builds.
Original change's description:
> Test mac system font variations.
>
> On macOS system fonts are special and sometimes have different behavior
> from fonts generated from data. Add a test which exercises several
> expectations about changing the variation on the system ui font.
>
> Bug: skia:10968
> Change-Id: Ia10dfbf7f4f0ff099f9bfebf95481c95c7d3715f
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372218
> Commit-Queue: Ben Wagner <bungeman@google.com>
> Reviewed-by: Herb Derby <herb@google.com>
TBR=bungeman@google.com,herb@google.com,drott@google.com
Change-Id: Iccc05f25d827ab85c507b5f3bde936561349e2b8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:10968
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372678
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
On macOS system fonts are special and sometimes have different behavior
from fonts generated from data. Add a test which exercises several
expectations about changing the variation on the system ui font.
Bug: skia:10968
Change-Id: Ia10dfbf7f4f0ff099f9bfebf95481c95c7d3715f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372218
Commit-Queue: Ben Wagner <bungeman@google.com>
Reviewed-by: Herb Derby <herb@google.com>
This detaches PromiseImages from any specific context, just to
a certain family.
Next up is to remove the tileSpecificSKP code from the DDLTileHelper.
Currently we have this janky PromiseImageDummy GrImageContext that
we make for each promise image. It's not ideal but it'll tide us over.
Bug: skia:10286
Change-Id: I12ab0bb7df9360a08af594da80de9df14cc2a44f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372516
Commit-Queue: Adlai Holler <adlai@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
There are two forms. Swizzle::Make supports components XYZW only;
Swizzle::MakeWith01 also supports the 01 components, and restructures
the zeros and ones into a constructor (as IRGenerator::convertSwizzle
has historically done). This means that once we are past the initial
IR generation stage, and we know that the 01 components have been
eliminated, we can avoid the extra 01-handling logic and just call
Swizzle::Make directly. This isn't a huge deal but it means that call
sites like the inliner can avoid some extra work that will never happen.
Change-Id: I46690c3d6b07feb6327ee72e8f66f15592a35554
Bug: skia:11342
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/371398
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Surprisingly, this error is actually caught by our parser, which
interprets the default label in a unique way. From the parser comments:
"Requiring default: to be last (in defiance of C and GLSL) was a
deliberate decision. Other parts of the compiler may rely upon this
assumption."
The comment is true--we don't check for duplicate default switch-case
labels anywhere else in the code, just here in the parser.
We rely on this, so we should have a test for it.
Change-Id: I6df5c565aca4d4b8565b96638dce9504efc39ccc
Bug: skia:11340
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372617
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
The IRGenerator's `convertConstructor` and `coerce` were tied at the
hip--coercion can create a constructor, and creating a constructor can
cause type-coercion. This CL migrates IRGenerator::coerce to
Type::coerceExpression, and migrates IRGenerator::convertConstructor to
Constructor::Make.
Most constructor creation should go through Constructor::Make instead of
make_unique<Constructor> for best results. There are exceptions to this
rule:
- during the Compiler's `optimize` phase, we hold raw pointers to
unique_ptrs of existing expression trees, and are manually tracking
variable usage counts, so adjusting the IR tree should be done with
extreme care. Continue to use make_unique here to avoid any "surprise
improvements."
- the Rehydrator is attempting to recreate an IR tree exactly as it used
to be and doesn't want additional optimization or fixups
There are still Constructor-related optimizations in simplifyExpression
which are not yet implemented in Constructor::Make. These are migrated
to Constructor::Make at http://review.skia.org/371482
Change-Id: I0f3876f932835fc2e347ae95414bc490085f120c
Bug: skia:11342
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370876
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Previously, a DSL unit test failure would not report the actual text
generated by the DSL, or the line number of the failure.
Added a new macro EXPECT_EQUAL which
- supports DSL statements, DSL expressions, and IR nodes equally
- reports both the expected string and actual DSL output
- reports the line number of the failure
- always compares in a whitespace-insensitive fashion
- supports passing rvalue DSL expressions directly, rather than forcing
us to create single-use lvalues for each test
Existing DSL tests have been updated to use this macro.
A failing test looks like this:
FAILURE: ../../tests/SkSLDSLTest.cpp:107 (Failed on line 146)
Expected: float4(0.0, 1.0, 2.0, 3.0)
Actual: float4(0.0, 1.0, 2.0, 4.0)
[DSLFloat, Mock]
Change-Id: Ie99d449690252f289bf48a66ab9c58e19dda9a07
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372198
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
The indirect tessellator can't combine overlapping, mismatched colors
because the log2 binning draws things out of order. But we can still
chain them together and generate a single long list of indirect draws.
Bug: chromium:1172543
Bug: skia:10419
Change-Id: Id7fc7488411a2a189e24cd7dd692e5c78497f498
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370197
Reviewed-by: Brian Salomon <bsalomon@google.com>
Only adds color to the hardware tessellator. The indirect tessellator
reorders draws with is log2 binning, so we can't have different
colors.
Bug: chromium:1172543
Bug: skia:10419
Change-Id: I2a3700cd4572e8222002bfb028af05c6ec447708
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/369976
Commit-Queue: Chris Dalton <csmartdalton@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I4df18946cdb3d9f1f7833461f913f2df94696821
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372197
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
These were all unused, and only implemented on one backend.
Change-Id: Ibd2fcef1a971e6c1bd9da0784c5d852a60708484
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372117
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Allows us to batch together strokes that have different SkStrokeRecs.
Bug: chromium:1172543
Bug: skia:10419
Change-Id: I11dc01e60bc17a6bb3c3b635f9edb2944a2f2edc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/369579
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Chris Dalton <csmartdalton@google.com>
Change-Id: I885149c73be63c223ac88a697ffe046a7f8384d0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372116
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Change-Id: I2bf8070fccacb21d2c5de56cdd9b6b77adb6c5a1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368876
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Bug: skia:11335
Change-Id: I88c952cbfe2d2c5920e17675da1674928f37b982
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/371480
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Bug: skia:11295
Change-Id: Iec11f3f4d26eb5b1c07707b3cedd09096bad80d0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/371478
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
When coercing a type, we would previously call checkValid() so we could
detect function-references and type-references, so we could get a nicer
error message.
It turns out that we can just do the "is this a type-reference/
function-reference?" check directly inside coerce() and get the same
improved error messages. Since we should be coercing all our values to
the right type, and type/function-references aren't coercible to
anything, this should catch them all. I don't expect any of these
to survive all the way to the end of IR generation.
(In case one of these types does slip through, I've left the error case
in checkValid, but I've also put in an assertion. If the fuzzer can
make that assertion fire, we are probably missing a call to coerce()
somewhere.)
This cleanup is meant to help migrate coerce() out of IRGenerator.
Change-Id: I031809adf439b1766048768b782c57e7f2494006
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/371479
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Adds trivial name mangling to the .stage output, so we can verify that
it's working in all places (declarations, references, etc). Also added
another global variable whose initializer is - in turn - another global.
Bug: skia:11295
Change-Id: Ic220bfae0a6d1eeeba66ade30d3d781af15c5dea
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/371477
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
The only color filter flag is isAlphaUnchanged(), so since alpha is
unchanged through any working format transformation, we can just return
whatever flags the child filter claims.
Add a simple test.
Change-Id: I2ce0300d9db55f953660ed157510070755c4c677
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370824
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
Change-Id: Idd0d49d3564dc3a24455db3c504ffa124f34dd05
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/371336
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Includes variables with and without initializers. Note that both the
.skvm and .stage output is incorrect right now. (No declarations for
global variables in .stage, and the initializer is dropped in .skvm).
Bug: skia:11295
Change-Id: Icb6d797616be6a1bc7cbdc9db4fefa7e30c65656
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/371143
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
None of these are legal in GLSL ES 1.0. Added a new test that previously
compiled without error. Started out with just assignment and equality,
then realized that sequence and ternary should be blocked, too.
Bug: skia:11323
Change-Id: I02691f819565afabeadbb12cab6c07acf40093f7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370880
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Simply build the skvm program and look to see if in.a.id == out.a.id.
This obviously is conservative, with false negatives possible. I
haven't done it here, but one possible improvement is to splat uniforms
so constant folding can peer through uniforms too.
Add some basic tests.
Change-Id: I5578ba38ff490b96bf84538025e08d9d352f0320
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370825
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This change will allow these types to be forward-declared; C++ doesn't
allow forward declaration of types declared inside a struct. Moving
these types out of Programs resulted in a large diff.
The Settings::Value helper class has been moved inside of the
IRGenerator. In practice, it was actually just an implementation detail
of how IRGenerator looks up caps-values by name. It seems very unlikely
that this will be necessary elsewhere going forward.
Change-Id: I6119417fae608f1c492a27de746d2b550ef8ca20
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370836
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 FreeType and CoreText ports would use the default value for an axis
instead of the current value for any unspecified axes. Change this so
that when cloning a typeface any unspecified axes preferentially use the
current axis value if it is known.
Also adds a test that unspecified axes are not changed when cloning.
Change-Id: I751ee5517f1d6b827c6d4ab245e9d681c8df6b42
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370456
Reviewed-by: Herb Derby <herb@google.com>
Reviewed-by: Dominik Röttsches <drott@chromium.org>
Commit-Queue: Ben Wagner <bungeman@google.com>
When SPIR-V generates function calls to an intrinsic, it assumes that
it can get a pointer to out-parameters referenced by the intrinsic.
This does not account for swizzled out-parameters; these are valid
lvalues, but do not work with getPointer().
The two intrinsics supported by SkSL which have an out-parameter are
frexp and modf, so these tests were fleshed out to trigger the error.
Neither of these are supported in ES2, though, so we cannot test them
via Runtime Effects.
Change-Id: Ib92707a28ba6d1c282d20e29a2a387bddf74ad23
Bug: skia:11052
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370116
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
The out-param helpers emitted by the Metal code gen (intended to provide
GLSL out-parameter semantics in Metal) emitted bad code if passed the
same variable for two separate out parameters. It would previously
create two parameters in the helper with the same name. The helper
function now omits the name of the second variable in the parameter list
if it is redundant; we already know the caller is passing the same
variable twice.
Change-Id: Ibdc6c02a9e9e4bdb4f4546a25068f2018aa07b10
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370258
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
GLSL ES2 documentation on out parameters: "Evaluation of an out
parameter results in an l-value that is used to copy out a value when
the function returns."
The inliner does not do any alias checking when inlining an `out` param.
That is, passing the same variable to two separate `out` parameters
would not generate two distinct lvalues in the inlined code; it reuses
the same variable for each out-params in the inlined code.
(Amusingly, our CFG can fully optimize away this test code so it just
returns "red".)
Change-Id: Ib781d2cfdac54f01b6abe159af0c84ff24ff6976
Bug: skia:11326
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370256
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Small diffs in a GM -- due to the dual nature of kMedium (cpu and gpu)
Another reason to stop using it, and switch to the more-explicit
sampling.
Bug: skia:7650
Change-Id: Ie7575071b19778626da7f94804abaaa11861a050
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370259
Reviewed-by: Mike Reed <reed@google.com>
Commit-Queue: Mike Reed <reed@google.com>
Multi-dimensional arrays aren't legal in GLSL/SkSL, so this should be
caught and flagged as an error. The parser now verifies that a
variable's type isn't an array-type before accepting a `[` token to
open an array on the variable name.
This CL also refactors the IR generator's `convertArraySize` method to
make sure that various checks are made for all callers. Originally this
restructuring was used to verify array multi-dimensionality, but that
didn't detect errors inside struct declarations (which get no error
checking inside the IR generator) so the IR generator updates no longer
need to check the array dimensions.
Bug: skia:11322
Change-Id: Id33f4bdfb544019ddf995a8196c3c09cfe5a4525
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/369916
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
We now interpret any statement of the form `Type identifier...` as a
var-declaration and report errors as such. Previously, if a var-decl
statement generated an error during parse, we'd report errors as if it
were an expression-statement, which meant that slightly-invalid code
could return out-of-context, misleading errors.
Bug: skia:11287
Change-Id: I2c6cf2984760eb34593c80cb30f8c4e007d42027
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370036
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Bug: skia:11314
Change-Id: I66476543462ae378a5bfb6cbd902dfa2f5fc45f5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/369917
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This reverts commit e89b50ae05.
Reason for revert: landed Android fix at http://ag/13544365 (master) and
http://ag/13554983 (sc-dev)
Original change's description:
> Android roll broke with a compilation error:
> frameworks/base/libs/hwui/jni/Shader.cpp:243:37: error: no matching function for call to 'get'
> sk_sp<SkRuntimeEffect> effect = std::get<0>(result)
>
> Revert "Remove deprecated form of SkRuntimeEffect::Make."
>
> This reverts commit 1cda194366.
>
> Reason for revert: <INSERT REASONING HERE>
>
> Original change's description:
> > Remove deprecated form of SkRuntimeEffect::Make.
> >
> > Chromium has migrated to the new API at https://crrev.com/c/2675855.
> >
> > Change-Id: Id4af77db2c462348e8031d28f56e543ad619c19c
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/367060
> > 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>
>
> TBR=mtklein@google.com,brianosman@google.com,johnstiles@google.com
>
> Change-Id: Ie18f865f3b7f5b0263db1e52b19cf6faa0500fdd
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368616
> Reviewed-by: Stan Iliev <stani@google.com>
> Commit-Queue: Stan Iliev <stani@google.com>
TBR=mtklein@google.com,brianosman@google.com,stani@google.com,johnstiles@google.com
Change-Id: I9d679013cb275dc80aaaa977b7f1f4da31f36d1e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/369037
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Derek Sollenberger <djsollen@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Submitting without additional flushing and syncing GPU/CPU is achievable
with public APIs. Moreover, this testing-only method bypasses code and
can lead to bugs.
Change-Id: I03bd0fa65e5ba88ba6181521947243ba84ab9696
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/369876
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
Move the contrast pin to Make() so it affects all implementations.
Bug: oss-fuzz:30859
Change-Id: Iaf7854701bc6a148976f653066d7bf8f27080d2f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/369698
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This allocator is made specifically to handle the memory after
the class of a textblob. It is lighter weight than ArenaAlloc,
easier to calculate needed memory.
Change-Id: Ie9f94e08e2ffd4041712dd3025296a830e940eb3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/356317
Commit-Queue: Herb Derby <herb@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
The CL at http://review.skia.org/366399 introduced a bug with
LValue::getPointer. Specifically, getPointer used to return zero when
no pointer is available. (This happens when the LValue is a swizzle.)
That CL changed the error code to -1. However, it did not fix up all
the call sites that checked the return value of getPointer().
This CL fixes up those call sites to use -1 consistently, and adds
TODOs in spots which do not check the result from getPointer() at all
(instead assuming it cannot fail). This will allow swizzled out-
parameters to work in SPIR-V as they did before. (Except in intrinsics,
where they seem to have been broken all along, but those are now marked
with a TODO at least.)
Note that we still do not fully emulate GLSL semantics for out
parameters, as out-parameters should only be copied back to the original
variable at the end of the function call to be fully GLSL compliant.
(This CL also replaces a tuple with a named struct for readability.)
Change-Id: I708dc7a69296a4244ba9ceb85c3e68d1f331bbc9
Bug: skia:11052
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368618
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Since review.skia.org/366716 this is unused and it makes our life
a whole lot simpler. And a little faster.
Bug: skia:10877
Change-Id: Ib7205bae57ce282f0e4f33c7c780a4c3e5159ea5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/369436
Commit-Queue: Adlai Holler <adlai@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
GrBagOfBytes parcels out bytes with a given size and alignment.
It is very careful about checking all values coming so that
no calculations overflows.
If the current block is not big enough to satisfy the request
bytes, a new block will be allocated to satisfy the
requested bytes.
Change-Id: If7b5e812f903b51ba4936605cd0dc298dda8821a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368117
Reviewed-by: Ben Wagner <bungeman@google.com>
Commit-Queue: Herb Derby <herb@google.com>
