Remove ShaderTranslatorTest.BuiltInFunctionEmulation

Remove ShaderTranslatorTest.BuiltInFunctionEmulation

This test becomes invalid with the upcoming angle changes. This test
expects webgle_dot_emu to be always there in translated shader source
but with the constant folding implemented in ANGLE for built-in
'dot', 'dot' function call will be replaced by actual result when all
the arguments are constant expressions and webgl_dot_emu will no
longer be there in translated shader source.

R=zmo

Committed: https://crrev.com/7dcae6d11a65967700eeaffecbd4bde6ad14c5f0
Cr-Commit-Position: refs/heads/master@{#333093}

[oetuaho-nv, 2015-06-05 07:54:03]

oetuaho@nvidia.com changed reviewers:
+ oetuaho@nvidia.com

[oetuaho-nv, 2015-06-05 07:54:03]

I think a better fix would be simply to change the shader so that it applies
dot() to a variable that is not constant, for example an uniform. That way we'll
still have the test coverage.

[apatole, 2015-06-05 08:22:49]

On 2015/06/05 07:54:03, oetuaho-nv wrote:
> I think a better fix would be simply to change the shader so that it applies
> dot() to a variable that is not constant, for example an uniform. That way
we'll
> still have the test coverage.

I am not sure about the importance of this test so I will wait for Mo's comment
on this.

For me, it looks OK to remove this test because of several reasons:
- It is specific to MAC, even for mac, it only covers 'dot' but now we have many
other functions emulated for mac as well. example: distance, length, normalize,
cos, etc.
- It covers only small part of emulation functionality that we have now. Just
tests whether webgl_dot_emu is present.
- Even if we need unit tests like these for functions like webgl_dot_emu,
angle_unittests looks better place.
- draw elements tests suite covers all these built-ins very well, so like other
GLSL built-ins, it looks OK to not have even angle_unittests for this built-in
as well.

[oetuaho-nv, 2015-06-05 08:48:46]

On 2015/06/05 08:22:49, apatole wrote:
> I am not sure about the importance of this test so I will wait for Mo's
comment
> on this.
> 
> For me, it looks OK to remove this test because of several reasons:
> - It is specific to MAC, even for mac, it only covers 'dot' but now we have
many
> other functions emulated for mac as well. example: distance, length,
normalize,
> cos, etc.
> - It covers only small part of emulation functionality that we have now. Just
> tests whether webgl_dot_emu is present.
> - Even if we need unit tests like these for functions like webgl_dot_emu,
> angle_unittests looks better place.
> - draw elements tests suite covers all these built-ins very well, so like
other
> GLSL built-ins, it looks OK to not have even angle_unittests for this built-in
> as well.

On another look, you're probably right that angle_unittests would be a better
place for this test. It verifies that when SH_EMULATE_BUILT_IN_FUNCTIONS is set,
the shader contains webgl_dot_emu. On first sight the "#ifdef MACOSX" suggests
this test might check that the workaround is enabled specifically for Mac, which
would be valuable, but that's not the case. In fact
SH_EMULATE_BUILT_IN_FUNCTIONS is always set in the test class regardless of OS.

dEQP tests on the other hand definitely don't have coverage for this - I don't
think anyone runs them in a configuration that would enable the Mac workarounds
that this test is testing.

[apatole, 2015-06-05 09:12:51]

On 2015/06/05 08:48:46, oetuaho-nv wrote:
> On 2015/06/05 08:22:49, apatole wrote:
> > I am not sure about the importance of this test so I will wait for Mo's
> comment
> > on this.
> > 
> > For me, it looks OK to remove this test because of several reasons:
> > - It is specific to MAC, even for mac, it only covers 'dot' but now we have
> many
> > other functions emulated for mac as well. example: distance, length,
> normalize,
> > cos, etc.
> > - It covers only small part of emulation functionality that we have now.
Just
> > tests whether webgl_dot_emu is present.
> > - Even if we need unit tests like these for functions like webgl_dot_emu,
> > angle_unittests looks better place.
> > - draw elements tests suite covers all these built-ins very well, so like
> other
> > GLSL built-ins, it looks OK to not have even angle_unittests for this
built-in
> > as well.
> 
> On another look, you're probably right that angle_unittests would be a better
> place for this test. It verifies that when SH_EMULATE_BUILT_IN_FUNCTIONS is
set,
> the shader contains webgl_dot_emu. On first sight the "#ifdef MACOSX" suggests
> this test might check that the workaround is enabled specifically for Mac,
which
> would be valuable, but that's not the case. In fact
> SH_EMULATE_BUILT_IN_FUNCTIONS is always set in the test class regardless of
OS.
> 
> dEQP tests on the other hand definitely don't have coverage for this - I don't
> think anyone runs them in a configuration that would enable the Mac
workarounds
> that this test is testing.

One clarification regarding dEQP tests coverage, sorry if it was not clear,
I was taking about overall built-in functions testing coverage in general and
not
about testing specific workarounds on specific platforms using dEQP.

[Jamie Madill, 2015-06-05 12:19:22]

jmadill@chromium.org changed reviewers:
+ jmadill@chromium.org

[Jamie Madill, 2015-06-05 12:19:22]

Fixing the test by using a varying or uniform seems pretty simple. OTOH yes, it
doesn't add much outside of additional ANGLE unit testing. I'm good with either
adding similar test to ANGLE (it's fine to have a very simple check, like this)
and removing the check or by changing the function to a uniform.

[Zhenyao Mo, 2015-06-05 18:06:34]

zmo@chromium.org changed reviewers:
+ zmo@chromium.org

[Zhenyao Mo, 2015-06-05 18:06:35]

LGTM

[Zhenyao Mo, 2015-06-05 18:06:58]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2015-06-05 18:07:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1161463013/20001

[Zhenyao Mo, 2015-06-05 18:13:30]

This test isn't robust at all.  It assumes the workaround is always applied on
Mac, which could easily change with new hardware for the bots.

This is definitely not a good place to test this behavior, so I go ahead and
land this to unblock apatole.

[commit-bot: I haz the power, 2015-06-05 18:22:50]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-05 18:35:11]

Patchset 2 (id:??) landed as
https://crrev.com/7dcae6d11a65967700eeaffecbd4bde6ad14c5f0
Cr-Commit-Position: refs/heads/master@{#333093}