Added Vulkan Shader Modules with GLSL->SPIR-V translation.

Added Vulkan Shader Modules with GLSL->SPIR-V translation.

R=piman@chromium.org
BUG=582558
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/d96a7978d1f33e981ca14c0d2da54eba4185ef3c
Cr-Commit-Position: refs/heads/master@{#394232}

[David Yen, 2016-05-13 23:42:48]

Description was changed from

==========
Added Vulkan Shader Modules with GLSL->SPIR-V translation.

R=piman@chromium.org
BUG=582558
==========

to

==========
Added Vulkan Shader Modules with GLSL->SPIR-V translation.

R=piman@chromium.org
BUG=582558
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[David Yen, 2016-05-16 22:27:38]

In case you started reviewing already, I just changed this class to be ref
counted. The reason is the pipeline class (next CL) will hold references to
these shader modules so it's just convenient for them to be ref counted so they
can be shared.

[David Yen, 2016-05-16 22:58:42]

Scratch the ref pointer idea... seems like lifetime management would be
ambiguous and bad. Probably better for pipeline to just store raw pointers and
have someone manage the lifetime of the shaders.

[piman, 2016-05-16 23:35:31]

https://codereview.chromium.org/1975183003/diff/80001/gpu/vulkan/vulkan_shade...
File gpu/vulkan/vulkan_shader_module.cc (right):

https://codereview.chromium.org/1975183003/diff/80001/gpu/vulkan/vulkan_shade...
gpu/vulkan/vulkan_shader_module.cc:14: class ShaderCCompiler {
Move this into an anonymous namespace, to avoid potential ODR issues.

https://codereview.chromium.org/1975183003/diff/80001/gpu/vulkan/vulkan_shade...
gpu/vulkan/vulkan_shader_module.cc:58: return
std::unique_ptr<ShaderCCompiler::CompilationResult>(
nit: you can use
base::MakeUnique<CompilationResult>(shaderc_compile_into_spv(...))

https://codereview.chromium.org/1975183003/diff/80001/gpu/vulkan/vulkan_shade...
gpu/vulkan/vulkan_shader_module.cc:115: aligned_source = entry_point;
Did you mean aligned_source = source?

https://codereview.chromium.org/1975183003/diff/80001/gpu/vulkan/vulkan_shade...
File gpu/vulkan/vulkan_shader_module.h (right):

https://codereview.chromium.org/1975183003/diff/80001/gpu/vulkan/vulkan_shade...
gpu/vulkan/vulkan_shader_module.h:21: INVALID = -1,
nit: do we need INVALID? e.g. IsValid is based on handle_ being !null. It would
be slightly more helpful to not have INVALID for things like switch cases.

https://codereview.chromium.org/1975183003/diff/80001/gpu/vulkan/vulkan_shade...
gpu/vulkan/vulkan_shader_module.h:30: const std::string& name,
nit: std::string (no const ref), and use std::move in the implementation (here
and below), and the callers if needed.

[David Yen, 2016-05-17 18:13:58]

https://codereview.chromium.org/1975183003/diff/80001/gpu/vulkan/vulkan_shade...
File gpu/vulkan/vulkan_shader_module.cc (right):

https://codereview.chromium.org/1975183003/diff/80001/gpu/vulkan/vulkan_shade...
gpu/vulkan/vulkan_shader_module.cc:14: class ShaderCCompiler {
On 2016/05/16 23:35:31, piman wrote:
> Move this into an anonymous namespace, to avoid potential ODR issues.

Done.

https://codereview.chromium.org/1975183003/diff/80001/gpu/vulkan/vulkan_shade...
gpu/vulkan/vulkan_shader_module.cc:58: return
std::unique_ptr<ShaderCCompiler::CompilationResult>(
On 2016/05/16 23:35:31, piman wrote:
> nit: you can use
> base::MakeUnique<CompilationResult>(shaderc_compile_into_spv(...))

Done.

https://codereview.chromium.org/1975183003/diff/80001/gpu/vulkan/vulkan_shade...
gpu/vulkan/vulkan_shader_module.cc:115: aligned_source = entry_point;
On 2016/05/16 23:35:31, piman wrote:
> Did you mean aligned_source = source?

Oops, done.

https://codereview.chromium.org/1975183003/diff/80001/gpu/vulkan/vulkan_shade...
File gpu/vulkan/vulkan_shader_module.h (right):

https://codereview.chromium.org/1975183003/diff/80001/gpu/vulkan/vulkan_shade...
gpu/vulkan/vulkan_shader_module.h:21: INVALID = -1,
On 2016/05/16 23:35:31, piman wrote:
> nit: do we need INVALID? e.g. IsValid is based on handle_ being !null. It
would
> be slightly more helpful to not have INVALID for things like switch cases.

Done.

[David Yen, 2016-05-17 18:14:17]

https://codereview.chromium.org/1975183003/diff/80001/gpu/vulkan/vulkan_shade...
File gpu/vulkan/vulkan_shader_module.h (right):

https://codereview.chromium.org/1975183003/diff/80001/gpu/vulkan/vulkan_shade...
gpu/vulkan/vulkan_shader_module.h:30: const std::string& name,
On 2016/05/16 23:35:31, piman wrote:
> nit: std::string (no const ref), and use std::move in the implementation (here
> and below), and the callers if needed.

Done.

[piman, 2016-05-17 18:53:59]

lgtm

https://codereview.chromium.org/1975183003/diff/100001/gpu/vulkan/vulkan_shad...
File gpu/vulkan/vulkan_shader_module.cc (right):

https://codereview.chromium.org/1975183003/diff/100001/gpu/vulkan/vulkan_shad...
gpu/vulkan/vulkan_shader_module.cc:121: for (int i = 0; i < (4 - source_mod);
++i) {
nit: actually if source.length() is already a multiple of 4 (which should be the
common case for code coming out of the compiler), source_mod will be 0, so you
will add 4 spaces - unfortunate because it means realloc.

you can maybe skip the whole loop if source_mod == 0? or use (3- (length-1)%4).

[David Yen, 2016-05-17 19:36:31]

https://codereview.chromium.org/1975183003/diff/100001/gpu/vulkan/vulkan_shad...
File gpu/vulkan/vulkan_shader_module.cc (right):

https://codereview.chromium.org/1975183003/diff/100001/gpu/vulkan/vulkan_shad...
gpu/vulkan/vulkan_shader_module.cc:121: for (int i = 0; i < (4 - source_mod);
++i) {
On 2016/05/17 18:53:59, piman wrote:
> nit: actually if source.length() is already a multiple of 4 (which should be
the
> common case for code coming out of the compiler), source_mod will be 0, so you
> will add 4 spaces - unfortunate because it means realloc.
> 
> you can maybe skip the whole loop if source_mod == 0? or use (3-
(length-1)%4).

That wouldn't work for 0 length sources though, I'll just check the edge
condition. Done.

[David Yen, 2016-05-17 20:05:04]

https://codereview.chromium.org/1975183003/diff/100001/gpu/vulkan/vulkan_shad...
File gpu/vulkan/vulkan_shader_module.cc (right):

https://codereview.chromium.org/1975183003/diff/100001/gpu/vulkan/vulkan_shad...
gpu/vulkan/vulkan_shader_module.cc:121: for (int i = 0; i < (4 - source_mod);
++i) {
On 2016/05/17 19:36:31, David Yen wrote:
> On 2016/05/17 18:53:59, piman wrote:
> > nit: actually if source.length() is already a multiple of 4 (which should be
> the
> > common case for code coming out of the compiler), source_mod will be 0, so
you
> > will add 4 spaces - unfortunate because it means realloc.
> > 
> > you can maybe skip the whole loop if source_mod == 0? or use (3-
> (length-1)%4).
> 
> That wouldn't work for 0 length sources though, I'll just check the edge
> condition. Done.

Although... if we really wanted to be branchless I suppose we could do (4 - (len
& 0x3) & 0x3), but that might be too cryptic. If you're fine with how it is now
I'll just leave it.

[piman, 2016-05-17 20:16:58]

On Tue, May 17, 2016 at 1:05 PM, <dyen@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1975183003/diff/100001/gpu/vulkan/vulkan_shad...
> File gpu/vulkan/vulkan_shader_module.cc (right):
>
>
>
https://codereview.chromium.org/1975183003/diff/100001/gpu/vulkan/vulkan_shad...
> gpu/vulkan/vulkan_shader_module.cc:121: for (int i = 0; i < (4 -
> source_mod); ++i) {
> On 2016/05/17 19:36:31, David Yen wrote:
> > On 2016/05/17 18:53:59, piman wrote:
> > > nit: actually if source.length() is already a multiple of 4 (which
> should be
> > the
> > > common case for code coming out of the compiler), source_mod will be
> 0, so you
> > > will add 4 spaces - unfortunate because it means realloc.
> > >
> > > you can maybe skip the whole loop if source_mod == 0? or use (3-
> > (length-1)%4).
> >
> > That wouldn't work for 0 length sources though, I'll just check the
> edge
> > condition. Done.
>
> Although... if we really wanted to be branchless I suppose we could do
> (4 - (len & 0x3) & 0x3), but that might be too cryptic. If you're fine
> with how it is now I'll just leave it.
>

It's ok, main thing is to avoid unnecessary reallocs. A branch here is in
the noise.


>
> https://codereview.chromium.org/1975183003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Yen, 2016-05-17 20:19:32]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2016-05-17 20:19:33]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1975183003/#ps120001
(title: "Check padding")

[commit-bot: I haz the power, 2016-05-17 20:20:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1975183003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1975183003/120001

[commit-bot: I haz the power, 2016-05-17 21:16:02]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-17 21:17:32]

Description was changed from

==========
Added Vulkan Shader Modules with GLSL->SPIR-V translation.

R=piman@chromium.org
BUG=582558
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Added Vulkan Shader Modules with GLSL->SPIR-V translation.

R=piman@chromium.org
BUG=582558
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/d96a7978d1f33e981ca14c0d2da54eba4185ef3c
Cr-Commit-Position: refs/heads/master@{#394232}
==========

[commit-bot: I haz the power, 2016-05-17 21:17:33]

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