Re: [Mesa-dev] [PATCH 1/2] ac/nir: set the DA field when performing atomics on 3D images

2018-02-21 Thread Samuel Pitoiset 



On 02/21/2018 09:24 AM, Timothy Arceri wrote:

On 21/02/18 19:22, Samuel Pitoiset wrote:

On 02/21/2018 05:11 AM, Timothy Arceri wrote:

On 21/02/18 07:29, Samuel Pitoiset wrote:
On VI, 3D images are considered as 2D arrays. RadeonSI sets DA for 
loads/stores/atomics and RADV only for loads/stores, so I guess 
there is a reason for that?


I've changed the nir->llvm code recently in order to fix some piglit 
test on the  radeonsi nir backend.


[1] 
https://cgit.freedesktop.org/mesa/mesa/commit/?id=e68150de263156a3f3d1b609b6506c5649967f61 

[2] 
https://cgit.freedesktop.org/mesa/mesa/commit/?id=82adf53308c137ce0dc5f2d5da4e7cc40c5b808c 




What was the chip?


Polaris I don't have a Vega for testing yet :)


So, that makes sense. :)










Anyway, there is a potential issue on the RADV side I think.

On 02/20/2018 04:43 PM, Nicolai Hähnle wrote:

Why? 3D images are not arrays.

On 20.02.2018 11:11, Samuel Pitoiset wrote:

This doesn't fix anything known but it should definitely be set.

Signed-off-by: Samuel Pitoiset 
---
  src/amd/common/ac_nir_to_llvm.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/amd/common/ac_nir_to_llvm.c 
b/src/amd/common/ac_nir_to_llvm.c

index dc471de977..9244f8bc7b 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -3764,7 +3764,8 @@ static LLVMValueRef 
visit_image_atomic(struct ac_nir_context *ctx,

  char coords_type[8];
  bool da = glsl_sampler_type_is_array(type) ||
-  glsl_get_sampler_dim(type) == 
GLSL_SAMPLER_DIM_CUBE;
+  glsl_get_sampler_dim(type) == 
GLSL_SAMPLER_DIM_CUBE ||

+  glsl_get_sampler_dim(type) == GLSL_SAMPLER_DIM_3D;
  LLVMValueRef coords = params[param_count++] = 
get_image_coords(ctx, instr);
  params[param_count++] = get_sampler_desc(ctx, 
instr->variables[0], AC_DESC_IMAGE,






___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] ac/nir: set the DA field when performing atomics on 3D images

2018-02-21 Thread Timothy Arceri 

On 21/02/18 19:22, Samuel Pitoiset wrote:

On 02/21/2018 05:11 AM, Timothy Arceri wrote:

On 21/02/18 07:29, Samuel Pitoiset wrote:
On VI, 3D images are considered as 2D arrays. RadeonSI sets DA for 
loads/stores/atomics and RADV only for loads/stores, so I guess there 
is a reason for that?


I've changed the nir->llvm code recently in order to fix some piglit 
test on the  radeonsi nir backend.


[1] 
https://cgit.freedesktop.org/mesa/mesa/commit/?id=e68150de263156a3f3d1b609b6506c5649967f61 

[2] 
https://cgit.freedesktop.org/mesa/mesa/commit/?id=82adf53308c137ce0dc5f2d5da4e7cc40c5b808c 



What was the chip?


Polaris I don't have a Vega for testing yet :)








Anyway, there is a potential issue on the RADV side I think.

On 02/20/2018 04:43 PM, Nicolai Hähnle wrote:

Why? 3D images are not arrays.

On 20.02.2018 11:11, Samuel Pitoiset wrote:

This doesn't fix anything known but it should definitely be set.

Signed-off-by: Samuel Pitoiset 
---
  src/amd/common/ac_nir_to_llvm.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/amd/common/ac_nir_to_llvm.c 
b/src/amd/common/ac_nir_to_llvm.c

index dc471de977..9244f8bc7b 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -3764,7 +3764,8 @@ static LLVMValueRef visit_image_atomic(struct 
ac_nir_context *ctx,

  char coords_type[8];
  bool da = glsl_sampler_type_is_array(type) ||
-  glsl_get_sampler_dim(type) == 
GLSL_SAMPLER_DIM_CUBE;
+  glsl_get_sampler_dim(type) == 
GLSL_SAMPLER_DIM_CUBE ||

+  glsl_get_sampler_dim(type) == GLSL_SAMPLER_DIM_3D;
  LLVMValueRef coords = params[param_count++] = 
get_image_coords(ctx, instr);
  params[param_count++] = get_sampler_desc(ctx, 
instr->variables[0], AC_DESC_IMAGE,






___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] ac/nir: set the DA field when performing atomics on 3D images

2018-02-21 Thread Samuel Pitoiset 



On 02/21/2018 05:11 AM, Timothy Arceri wrote:

On 21/02/18 07:29, Samuel Pitoiset wrote:
On VI, 3D images are considered as 2D arrays. RadeonSI sets DA for 
loads/stores/atomics and RADV only for loads/stores, so I guess there 
is a reason for that?


I've changed the nir->llvm code recently in order to fix some piglit 
test on the  radeonsi nir backend.


[1] 
https://cgit.freedesktop.org/mesa/mesa/commit/?id=e68150de263156a3f3d1b609b6506c5649967f61 

[2] 
https://cgit.freedesktop.org/mesa/mesa/commit/?id=82adf53308c137ce0dc5f2d5da4e7cc40c5b808c 


What was the chip?






Anyway, there is a potential issue on the RADV side I think.

On 02/20/2018 04:43 PM, Nicolai Hähnle wrote:

Why? 3D images are not arrays.

On 20.02.2018 11:11, Samuel Pitoiset wrote:

This doesn't fix anything known but it should definitely be set.

Signed-off-by: Samuel Pitoiset 
---
  src/amd/common/ac_nir_to_llvm.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/amd/common/ac_nir_to_llvm.c 
b/src/amd/common/ac_nir_to_llvm.c

index dc471de977..9244f8bc7b 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -3764,7 +3764,8 @@ static LLVMValueRef visit_image_atomic(struct 
ac_nir_context *ctx,

  char coords_type[8];
  bool da = glsl_sampler_type_is_array(type) ||
-  glsl_get_sampler_dim(type) == GLSL_SAMPLER_DIM_CUBE;
+  glsl_get_sampler_dim(type) == 
GLSL_SAMPLER_DIM_CUBE ||

+  glsl_get_sampler_dim(type) == GLSL_SAMPLER_DIM_3D;
  LLVMValueRef coords = params[param_count++] = 
get_image_coords(ctx, instr);
  params[param_count++] = get_sampler_desc(ctx, 
instr->variables[0], AC_DESC_IMAGE,






___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] ac/nir: set the DA field when performing atomics on 3D images

2018-02-20 Thread Timothy Arceri 

On 21/02/18 07:29, Samuel Pitoiset wrote:
On VI, 3D images are considered as 2D arrays. RadeonSI sets DA for 
loads/stores/atomics and RADV only for loads/stores, so I guess there is 
a reason for that?


I've changed the nir->llvm code recently in order to fix some piglit 
test on the  radeonsi nir backend.


[1] 
https://cgit.freedesktop.org/mesa/mesa/commit/?id=e68150de263156a3f3d1b609b6506c5649967f61
[2] 
https://cgit.freedesktop.org/mesa/mesa/commit/?id=82adf53308c137ce0dc5f2d5da4e7cc40c5b808c




Anyway, there is a potential issue on the RADV side I think.

On 02/20/2018 04:43 PM, Nicolai Hähnle wrote:

Why? 3D images are not arrays.

On 20.02.2018 11:11, Samuel Pitoiset wrote:

This doesn't fix anything known but it should definitely be set.

Signed-off-by: Samuel Pitoiset 
---
  src/amd/common/ac_nir_to_llvm.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/amd/common/ac_nir_to_llvm.c 
b/src/amd/common/ac_nir_to_llvm.c

index dc471de977..9244f8bc7b 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -3764,7 +3764,8 @@ static LLVMValueRef visit_image_atomic(struct 
ac_nir_context *ctx,

  char coords_type[8];
  bool da = glsl_sampler_type_is_array(type) ||
-  glsl_get_sampler_dim(type) == GLSL_SAMPLER_DIM_CUBE;
+  glsl_get_sampler_dim(type) == 
GLSL_SAMPLER_DIM_CUBE ||

+  glsl_get_sampler_dim(type) == GLSL_SAMPLER_DIM_3D;
  LLVMValueRef coords = params[param_count++] = 
get_image_coords(ctx, instr);
  params[param_count++] = get_sampler_desc(ctx, 
instr->variables[0], AC_DESC_IMAGE,






___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] ac/nir: set the DA field when performing atomics on 3D images

2018-02-20 Thread Samuel Pitoiset 



On 02/20/2018 09:45 PM, Marek Olšák wrote:

On Tue, Feb 20, 2018 at 9:29 PM, Samuel Pitoiset
 wrote:

On VI, 3D images are considered as 2D arrays. RadeonSI sets DA for
loads/stores/atomics and RADV only for loads/stores, so I guess there is a
reason for that?


I don't really know why radeonsi sets DA=1 for 3D images on GFX9. I
guess the code was inherited from previous chips.


Yeah, that makes sense.



On VI and older, 3D images are bound as 2D arrays.

Marek


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] ac/nir: set the DA field when performing atomics on 3D images

2018-02-20 Thread Marek Olšák 
On Tue, Feb 20, 2018 at 9:29 PM, Samuel Pitoiset
 wrote:
> On VI, 3D images are considered as 2D arrays. RadeonSI sets DA for
> loads/stores/atomics and RADV only for loads/stores, so I guess there is a
> reason for that?

I don't really know why radeonsi sets DA=1 for 3D images on GFX9. I
guess the code was inherited from previous chips.

On VI and older, 3D images are bound as 2D arrays.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] ac/nir: set the DA field when performing atomics on 3D images

2018-02-20 Thread Samuel Pitoiset 
On VI, 3D images are considered as 2D arrays. RadeonSI sets DA for 
loads/stores/atomics and RADV only for loads/stores, so I guess there is 
a reason for that?


Anyway, there is a potential issue on the RADV side I think.

On 02/20/2018 04:43 PM, Nicolai Hähnle wrote:

Why? 3D images are not arrays.

On 20.02.2018 11:11, Samuel Pitoiset wrote:

This doesn't fix anything known but it should definitely be set.

Signed-off-by: Samuel Pitoiset 
---
  src/amd/common/ac_nir_to_llvm.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/amd/common/ac_nir_to_llvm.c 
b/src/amd/common/ac_nir_to_llvm.c

index dc471de977..9244f8bc7b 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -3764,7 +3764,8 @@ static LLVMValueRef visit_image_atomic(struct 
ac_nir_context *ctx,

  char coords_type[8];
  bool da = glsl_sampler_type_is_array(type) ||
-  glsl_get_sampler_dim(type) == GLSL_SAMPLER_DIM_CUBE;
+  glsl_get_sampler_dim(type) == GLSL_SAMPLER_DIM_CUBE ||
+  glsl_get_sampler_dim(type) == GLSL_SAMPLER_DIM_3D;
  LLVMValueRef coords = params[param_count++] = 
get_image_coords(ctx, instr);
  params[param_count++] = get_sampler_desc(ctx, 
instr->variables[0], AC_DESC_IMAGE,






___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] ac/nir: set the DA field when performing atomics on 3D images

2018-02-20 Thread Marek Olšák 
On Tue, Feb 20, 2018 at 4:43 PM, Nicolai Hähnle  wrote:
> Why? 3D images are not arrays.

radeonsi sets DA=1 too.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] ac/nir: set the DA field when performing atomics on 3D images

2018-02-20 Thread Nicolai Hähnle 

Why? 3D images are not arrays.

On 20.02.2018 11:11, Samuel Pitoiset wrote:

This doesn't fix anything known but it should definitely be set.

Signed-off-by: Samuel Pitoiset 
---
  src/amd/common/ac_nir_to_llvm.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
index dc471de977..9244f8bc7b 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -3764,7 +3764,8 @@ static LLVMValueRef visit_image_atomic(struct 
ac_nir_context *ctx,
char coords_type[8];
  
  		bool da = glsl_sampler_type_is_array(type) ||

- glsl_get_sampler_dim(type) == GLSL_SAMPLER_DIM_CUBE;
+ glsl_get_sampler_dim(type) == GLSL_SAMPLER_DIM_CUBE ||
+ glsl_get_sampler_dim(type) == GLSL_SAMPLER_DIM_3D;
  
  		LLVMValueRef coords = params[param_count++] = get_image_coords(ctx, instr);

params[param_count++] = get_sampler_desc(ctx, 
instr->variables[0], AC_DESC_IMAGE,




--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/2] ac/nir: set the DA field when performing atomics on 3D images

2018-02-20 Thread Samuel Pitoiset 
This doesn't fix anything known but it should definitely be set.

Signed-off-by: Samuel Pitoiset 
---
 src/amd/common/ac_nir_to_llvm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
index dc471de977..9244f8bc7b 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -3764,7 +3764,8 @@ static LLVMValueRef visit_image_atomic(struct 
ac_nir_context *ctx,
char coords_type[8];
 
bool da = glsl_sampler_type_is_array(type) ||
- glsl_get_sampler_dim(type) == GLSL_SAMPLER_DIM_CUBE;
+ glsl_get_sampler_dim(type) == GLSL_SAMPLER_DIM_CUBE ||
+ glsl_get_sampler_dim(type) == GLSL_SAMPLER_DIM_3D;
 
LLVMValueRef coords = params[param_count++] = 
get_image_coords(ctx, instr);
params[param_count++] = get_sampler_desc(ctx, 
instr->variables[0], AC_DESC_IMAGE,
-- 
2.16.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev