Discussion:
[PATCH 1/2] radv: enable lowering of nir_op_bitfield_insert
Add Reply
Samuel Pitoiset
2017-12-05 17:50:21 UTC
Reply
Permalink
Raw Message
Otherwise it's replaced by
"vec1 32 ssa_108 = load_const (0x00000000 /* 0.000000 */)", which
looks clearly wrong.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104119
Signed-off-by: Samuel Pitoiset <***@gmail.com>
---
src/amd/vulkan/radv_shader.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/src/amd/vulkan/radv_shader.c b/src/amd/vulkan/radv_shader.c
index 4a3fdfa80e..0b19d23fa2 100644
--- a/src/amd/vulkan/radv_shader.c
+++ b/src/amd/vulkan/radv_shader.c
@@ -66,6 +66,7 @@ static const struct nir_shader_compiler_options nir_options = {
.lower_extract_byte = true,
.lower_extract_word = true,
.lower_ffma = true,
+ .lower_bitfield_insert = true,
.max_unroll_iterations = 32
};
--
2.15.1
Samuel Pitoiset
2017-12-05 17:50:22 UTC
Reply
Permalink
Raw Message
This instruction should also be lowered correctly.

Signed-off-by: Samuel Pitoiset <***@gmail.com>
---
src/amd/vulkan/radv_shader.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/src/amd/vulkan/radv_shader.c b/src/amd/vulkan/radv_shader.c
index 0b19d23fa2..044bcd0641 100644
--- a/src/amd/vulkan/radv_shader.c
+++ b/src/amd/vulkan/radv_shader.c
@@ -67,6 +67,7 @@ static const struct nir_shader_compiler_options nir_options = {
.lower_extract_word = true,
.lower_ffma = true,
.lower_bitfield_insert = true,
+ .lower_bitfield_extract = true,
.max_unroll_iterations = 32
};
--
2.15.1
Connor Abbott
2017-12-05 19:24:55 UTC
Reply
Permalink
Raw Message
Same comment as the previous patch.

On Tue, Dec 5, 2017 at 12:50 PM, Samuel Pitoiset
Post by Samuel Pitoiset
This instruction should also be lowered correctly.
---
src/amd/vulkan/radv_shader.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/amd/vulkan/radv_shader.c b/src/amd/vulkan/radv_shader.c
index 0b19d23fa2..044bcd0641 100644
--- a/src/amd/vulkan/radv_shader.c
+++ b/src/amd/vulkan/radv_shader.c
@@ -67,6 +67,7 @@ static const struct nir_shader_compiler_options nir_options = {
.lower_extract_word = true,
.lower_ffma = true,
.lower_bitfield_insert = true,
+ .lower_bitfield_extract = true,
.max_unroll_iterations = 32
};
--
2.15.1
_______________________________________________
mesa-dev mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Connor Abbott
2017-12-05 19:24:07 UTC
Reply
Permalink
Raw Message
lower_bitfield_insert lowers nir_op_bitfield_insert to DX10-style
nir_op_bfi and nir_op_bfm, both of which aren't handled by
ac_nir_to_llvm, so unless I'm missing something this will just break
them even harder. We probably should use this lowering after adding
support for bfi and bfm, since AMD does have native instructions for
bfi and bfm, but first I'd like to see the actual bug fixed. Have you
tried running it with NIR_PRINT=true to pin down which optimization
pass is going wrong?

On Tue, Dec 5, 2017 at 12:50 PM, Samuel Pitoiset
Post by Samuel Pitoiset
Otherwise it's replaced by
"vec1 32 ssa_108 = load_const (0x00000000 /* 0.000000 */)", which
looks clearly wrong.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104119
---
src/amd/vulkan/radv_shader.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/amd/vulkan/radv_shader.c b/src/amd/vulkan/radv_shader.c
index 4a3fdfa80e..0b19d23fa2 100644
--- a/src/amd/vulkan/radv_shader.c
+++ b/src/amd/vulkan/radv_shader.c
@@ -66,6 +66,7 @@ static const struct nir_shader_compiler_options nir_options = {
.lower_extract_byte = true,
.lower_extract_word = true,
.lower_ffma = true,
+ .lower_bitfield_insert = true,
.max_unroll_iterations = 32
};
--
2.15.1
_______________________________________________
mesa-dev mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
James Legg
2017-12-06 12:04:21 UTC
Reply
Permalink
Raw Message
Post by Connor Abbott
lower_bitfield_insert lowers nir_op_bitfield_insert to DX10-style
nir_op_bfi and nir_op_bfm, both of which aren't handled by
ac_nir_to_llvm, so unless I'm missing something this will just break
them even harder. We probably should use this lowering after adding
support for bfi and bfm, since AMD does have native instructions for
bfi and bfm, but first I'd like to see the actual bug fixed. Have you
tried running it with NIR_PRINT=true to pin down which optimization
pass is going wrong?
I've identified the constant folding pass as the culprit and fixed the
incorrect logic for bitfield_insert with this patch:
nir/opcodes: Fix constant-folding of bitfield_insert
https://patchwork.freedesktop.org/patch/191977/

James
Samuel Pitoiset
2017-12-06 12:47:44 UTC
Reply
Permalink
Raw Message
Post by James Legg
Post by Connor Abbott
lower_bitfield_insert lowers nir_op_bitfield_insert to DX10-style
nir_op_bfi and nir_op_bfm, both of which aren't handled by
ac_nir_to_llvm, so unless I'm missing something this will just break
them even harder. We probably should use this lowering after adding
support for bfi and bfm, since AMD does have native instructions for
bfi and bfm, but first I'd like to see the actual bug fixed. Have you
tried running it with NIR_PRINT=true to pin down which optimization
pass is going wrong?
I've identified the constant folding pass as the culprit and fixed the
nir/opcodes: Fix constant-folding of bitfield_insert
https://patchwork.freedesktop.org/patch/191977/
Your fix looks much better, thanks!
Post by James Legg
James
Loading...