[Mesa-dev] [PATCH 1/2] intel/fs/bank_conflicts: Use posix_memalign() instead of overaligned new to obtain vector storage.

[Ian Romanick]

Series is

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

On 12/18/2017 03:26 PM, Francisco Jerez wrote:
> The weight_vector_type constructor was inadvertently assuming C++17
> semantics of the new operator applied on a type with alignment
> requirement greater than the largest fundamental alignment.
> Unfortunately on earlier C++ dialects the implementation was allowed
> to raise an allocation failure when the alignment requirement of the
> allocated type was unsupported, in an implementation-defined fashion.
> It's expected that a C++ implementation recent enough to implement
> P0035R4 would have honored allocation requests for such over-aligned
> types even if the C++17 dialect wasn't active, which is likely the
> reason why this problem wasn't caught by our CI system.
> 
> A more elegant fix would involve wrapping the __SSE2__ block in a
> '__cpp_aligned_new >= 201606' preprocessor conditional and continue
> taking advantage of the language feature, but that would yield lower
> compile-time performance on old compilers not implementing it
> (e.g. GCC versions older than 7.0).
> 
> Fixes: af2c320190f3c731 "intel/fs: Implement GRF bank conflict mitigation pass."
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104226
> Reported-by: Józef Kucia <joseph.kucia at gmail.com>
> ---
>  src/intel/compiler/brw_fs_bank_conflicts.cpp | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/src/intel/compiler/brw_fs_bank_conflicts.cpp b/src/intel/compiler/brw_fs_bank_conflicts.cpp
> index 0cd880d44f2..e87fcbfc5eb 100644
> --- a/src/intel/compiler/brw_fs_bank_conflicts.cpp
> +++ b/src/intel/compiler/brw_fs_bank_conflicts.cpp
> @@ -277,13 +277,10 @@ namespace {
>     struct weight_vector_type {
>        weight_vector_type() : v(NULL), size(0) {}
>  
> -      weight_vector_type(unsigned n) :
> -         v(new vector_type[DIV_ROUND_UP(n, vector_width)]()),
> -         size(n) {}
> +      weight_vector_type(unsigned n) : v(alloc(n)), size(n) {}
>  
>        weight_vector_type(const weight_vector_type &u) :
> -         v(new vector_type[DIV_ROUND_UP(u.size, vector_width)]()),
> -         size(u.size)
> +         v(alloc(u.size)), size(u.size)
>        {
>           memcpy(v, u.v,
>                  DIV_ROUND_UP(u.size, vector_width) * sizeof(vector_type));
> @@ -291,7 +288,7 @@ namespace {
>  
>        ~weight_vector_type()
>        {
> -         delete[] v;
> +         free(v);
>        }
>  
>        weight_vector_type &
> @@ -304,6 +301,19 @@ namespace {
>  
>        vector_type *v;
>        unsigned size;
> +
> +   private:
> +      static vector_type *
> +      alloc(unsigned n)
> +      {
> +         const unsigned align = MAX2(sizeof(void *), __alignof__(vector_type));
> +         const unsigned size = DIV_ROUND_UP(n, vector_width) * sizeof(vector_type);
> +         void *p;
> +         if (posix_memalign(&p, align, size))
> +            return NULL;
> +         memset(p, 0, size);
> +         return reinterpret_cast<vector_type *>(p);
> +      }
>     };
>  
>     /**
>