[Mesa-dev] [PATCH v2 56/59] i965/fs: push first double-based uniforms in push constant buffer

[Francisco Jerez]

Kenneth Graunke <kenneth at whitecape.org> writes:

> On Monday, May 9, 2016 7:33:02 AM PDT Samuel Iglesias Gonsálvez wrote:
>> 
>> On 09/05/16 07:21, Pohjolainen, Topi wrote:
>> > On Fri, May 06, 2016 at 08:56:08AM +0200, Samuel Iglesias Gons?lvez wrote:
>> >> When there is a mix of definitions of uniforms with 32-bit or 64-bit
>> >> data type sizes, the driver ends up doing misaligned access to double
>> >> based variables in the push constant buffer.
>> >>
>> >> To fix this, this patch pushes first all the 64-bit variables and
>> >> then the rest. Then, all the variables would be aligned to
>> >> its data type size.
>> >>
>> >> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
>> >> ---
>> >>  src/mesa/drivers/dri/i965/brw_fs.cpp | 113 ++++++++++++++++++++++++
> +----------
>> >>  1 file changed, 83 insertions(+), 30 deletions(-)
>> >>
>> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/
> i965/brw_fs.cpp
>> >> index 65aa3c7..7eaf1b4 100644
>> >> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> >> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> >> @@ -39,6 +39,7 @@
>> >>  #include "brw_program.h"
>> >>  #include "brw_dead_control_flow.h"
>> >>  #include "compiler/glsl_types.h"
>> >> +#include "program/prog_parameter.h"
>> >>  
>> >>  using namespace brw;
>> >>  
>> >> @@ -2004,6 +2005,45 @@ fs_visitor::compact_virtual_grfs()
>> >>     return progress;
>> >>  }
>> >>  
>> >> +static void
>> >> +set_push_pull_constant_loc(unsigned uniform, int &chunk_start, bool 
> contiguous,
>> >> +                           int *push_constant_loc, int 
> *pull_constant_loc,
>> >> +                           unsigned &num_push_constants,
>> >> +                           unsigned &num_pull_constants,
>> > 
>> > I don't remember us using writable references elsewhere, and vaguely 
> recall
>> > there being a debate about this and decision to use pointers instead?
>> > 
>> 
>> OK, I will change it to use pointers instead. I had no idea this was
>> discussed before.
>
> Oh, good catch.  We usually avoid pass-by-reference unless it's const,
> and instead return out parameters via pointers.
>
> Paul Berry, Chris Forbes, and I discussed this briefly in August 2013.
> I'd suggested passing by reference, and both Paul and Chris preferred
> pointers.  I believe a number of other developers expressed a preference
> for pointers on IRC / in-person as well.
>

If you ask me what I think about this rule, I consider read/write
parameters kind of an anti-pattern either way (in modern C++ they're
*almost* never a necessity), regardless of whether you use pointers or
references to pass by reference, except in cases where the function you
are defining is inherently mutating on some of its arguments (and by
"inherently" I mean they couldn't possibly be misinterpreted as input
arguments by the reader), like:

   void optimize(shader &);
   void swap(thing &, thing &);
   void set_property(thing &, value);
   void sort_inplace(list &);
   bool try_lock(mutex &);
   number &operator+=(number &, number);

The above is highly idiomatic C++ (heck, the last one is even built into
the language :P), and because the argument mutation is already explicit
there is no benefit from making it explicit twice -- Quite the opposite
I would consider it clumsy and pedantic to pass the object by pointer in
any of the examples above, and you'd lose the non-nullability and (lack
of) ownership transfer semantics of reference types which is what the
distinction between pointers and references is typically based on in C++
code -- For that reason I wouldn't feel comfortable with a rule banning
mutable references from being used universally.

> https://lists.freedesktop.org/archives/mesa-dev/2013-August/043593.html
>
> I certainly wouldn't expect you to have seen that obscure discussion :)
>
> --Ken
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160509/7316d72d/attachment.sig>