Discussion:
[PATCH 2/2] spirv: fix bug when OpSpecConstantOp calls a conversion
Add Reply
Samuel Iglesias Gonsálvez
2017-11-21 06:25:53 UTC
Reply
Permalink
Raw Message
In that case, nir_eval_const_opcode() will evaluate the conversion
but as it was using destination's bit_size, the resulting
value was just a cast of the source constant value. By passing the
source's bit size, it does the conversion properly.

Fixes:

dEQP-VK.spirv_assembly.instruction.*.opspecconstantop.*convert*

Signed-off-by: Samuel Iglesias Gonsálvez <***@igalia.com>
---
src/compiler/spirv/spirv_to_nir.c | 31 ++++++++++++++++++++++++++-----
1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c
index 2cc3c275ea9..ffbda4f1946 100644
--- a/src/compiler/spirv/spirv_to_nir.c
+++ b/src/compiler/spirv/spirv_to_nir.c
@@ -1348,14 +1348,35 @@ vtn_handle_constant(struct vtn_builder *b, SpvOp opcode,
bool swap;
nir_alu_type dst_alu_type = nir_get_nir_type_for_glsl_type(val->const_type);
nir_alu_type src_alu_type = dst_alu_type;
- nir_op op = vtn_nir_alu_op_for_spirv_opcode(opcode, &swap, src_alu_type, dst_alu_type);
-
unsigned num_components = glsl_get_vector_elements(val->const_type);
- unsigned bit_size =
- glsl_get_bit_size(val->const_type);
+ unsigned bit_size;

- nir_const_value src[4];
assert(count <= 7);
+
+ switch (opcode) {
+ case SpvOpUConvert:
+ case SpvOpConvertFToU:
+ case SpvOpConvertFToS:
+ case SpvOpConvertSToF:
+ case SpvOpConvertUToF:
+ case SpvOpSConvert:
+ case SpvOpFConvert:
+ /* We have a source in a conversion */
+ src_alu_type =
+ nir_get_nir_type_for_glsl_type(
+ vtn_value(b, w[4], vtn_value_type_constant)->const_type);
+ /* We use the bitsize of the conversion source to evaluate the opcode later */
+ bit_size = glsl_get_bit_size(
+ vtn_value(b, w[4], vtn_value_type_constant)->const_type);
+ break;
+ default:
+ bit_size = glsl_get_bit_size(val->const_type);
+ };
+
+
+ nir_op op = vtn_nir_alu_op_for_spirv_opcode(opcode, &swap, src_alu_type, dst_alu_type);
+ nir_const_value src[4];
+
for (unsigned i = 0; i < count - 4; i++) {
nir_constant *c =
vtn_value(b, w[4 + i], vtn_value_type_constant)->constant;
--
2.14.3
Samuel Iglesias Gonsálvez
2017-11-28 10:05:32 UTC
Reply
Permalink
Raw Message
This patch series is still unreviewed.

Sam
---
src/compiler/spirv/spirv_to_nir.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/compiler/spirv/spirv_to_nir.c
b/src/compiler/spirv/spirv_to_nir.c
index 027efab88d7..2cc3c275ea9 100644
--- a/src/compiler/spirv/spirv_to_nir.c
+++ b/src/compiler/spirv/spirv_to_nir.c
@@ -1361,7 +1361,6 @@ vtn_handle_constant(struct vtn_builder *b,
SpvOp opcode,
vtn_value(b, w[4 + i], vtn_value_type_constant)-
Post by Samuel Iglesias Gonsálvez
constant;
unsigned j = swap ? 1 - i : i;
- assert(bit_size == 32);
src[j] = c->values[0];
}
Ian Romanick
2017-11-28 21:10:41 UTC
Reply
Permalink
Raw Message
This patch is
---
src/compiler/spirv/spirv_to_nir.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c
index 027efab88d7..2cc3c275ea9 100644
--- a/src/compiler/spirv/spirv_to_nir.c
+++ b/src/compiler/spirv/spirv_to_nir.c
@@ -1361,7 +1361,6 @@ vtn_handle_constant(struct vtn_builder *b, SpvOp opcode,
vtn_value(b, w[4 + i], vtn_value_type_constant)->constant;
unsigned j = swap ? 1 - i : i;
- assert(bit_size == 32);
src[j] = c->values[0];
}
Ian Romanick
2017-11-28 21:13:50 UTC
Reply
Permalink
Raw Message
Post by Samuel Iglesias Gonsálvez
In that case, nir_eval_const_opcode() will evaluate the conversion
but as it was using destination's bit_size, the resulting
value was just a cast of the source constant value. By passing the
source's bit size, it does the conversion properly.
dEQP-VK.spirv_assembly.instruction.*.opspecconstantop.*convert*
---
src/compiler/spirv/spirv_to_nir.c | 31 ++++++++++++++++++++++++++-----
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c
index 2cc3c275ea9..ffbda4f1946 100644
--- a/src/compiler/spirv/spirv_to_nir.c
+++ b/src/compiler/spirv/spirv_to_nir.c
@@ -1348,14 +1348,35 @@ vtn_handle_constant(struct vtn_builder *b, SpvOp opcode,
bool swap;
nir_alu_type dst_alu_type = nir_get_nir_type_for_glsl_type(val->const_type);
nir_alu_type src_alu_type = dst_alu_type;
- nir_op op = vtn_nir_alu_op_for_spirv_opcode(opcode, &swap, src_alu_type, dst_alu_type);
-
unsigned num_components = glsl_get_vector_elements(val->const_type);
- unsigned bit_size =
- glsl_get_bit_size(val->const_type);
+ unsigned bit_size;
- nir_const_value src[4];
assert(count <= 7);
+
+ switch (opcode) {
I'm not sure what we should do here. Most of these opcodes are not
valid in SpvOpSpecConstantOp in a Shader. Only SpvOpSConvert and
SpvOpFConvert are allowed. I guess it's trivial enough to support the
others anyway... do those dEQP-VK.spirv_assembly.instruction.* tests
exercise the other opcodes?
Post by Samuel Iglesias Gonsálvez
+ /* We have a source in a conversion */
+ src_alu_type =
+ nir_get_nir_type_for_glsl_type(
+ vtn_value(b, w[4], vtn_value_type_constant)->const_type);
+ /* We use the bitsize of the conversion source to evaluate the opcode later */
+ bit_size = glsl_get_bit_size(
+ vtn_value(b, w[4], vtn_value_type_constant)->const_type);
+ break;
+ bit_size = glsl_get_bit_size(val->const_type);
+ };
+
+
+ nir_op op = vtn_nir_alu_op_for_spirv_opcode(opcode, &swap, src_alu_type, dst_alu_type);
+ nir_const_value src[4];
+
for (unsigned i = 0; i < count - 4; i++) {
nir_constant *c =
vtn_value(b, w[4 + i], vtn_value_type_constant)->constant;
Samuel Iglesias Gonsálvez
2017-11-29 07:17:03 UTC
Reply
Permalink
Raw Message
Post by Ian Romanick
Post by Samuel Iglesias Gonsálvez
In that case, nir_eval_const_opcode() will evaluate the conversion
but as it was using destination's bit_size, the resulting
value was just a cast of the source constant value. By passing the
source's bit size, it does the conversion properly.
dEQP-VK.spirv_assembly.instruction.*.opspecconstantop.*convert*
---
src/compiler/spirv/spirv_to_nir.c | 31 ++++++++++++++++++++++++++-
----
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/src/compiler/spirv/spirv_to_nir.c
b/src/compiler/spirv/spirv_to_nir.c
index 2cc3c275ea9..ffbda4f1946 100644
--- a/src/compiler/spirv/spirv_to_nir.c
+++ b/src/compiler/spirv/spirv_to_nir.c
@@ -1348,14 +1348,35 @@ vtn_handle_constant(struct vtn_builder *b, SpvOp opcode,
bool swap;
nir_alu_type dst_alu_type =
nir_get_nir_type_for_glsl_type(val->const_type);
nir_alu_type src_alu_type = dst_alu_type;
- nir_op op = vtn_nir_alu_op_for_spirv_opcode(opcode,
&swap, src_alu_type, dst_alu_type);
-
unsigned num_components = glsl_get_vector_elements(val-
Post by Samuel Iglesias Gonsálvez
const_type);
- unsigned bit_size =
- glsl_get_bit_size(val->const_type);
+ unsigned bit_size;
- nir_const_value src[4];
assert(count <= 7);
+
+ switch (opcode) {
I'm not sure what we should do here. Most of these opcodes are not
valid in SpvOpSpecConstantOp in a Shader. Only SpvOpSConvert and
SpvOpFConvert are allowed. I guess it's trivial enough to support the
others anyway...
Right.
Post by Ian Romanick
do those dEQP-VK.spirv_assembly.instruction.* tests
exercise the other opcodes?
No, they only exercise SpvOpFConvert and SpvOpSConvert. Do you prefer
to just support these two opcodes for the moment?

With or without that change, does it get your R-b?

Sam
Post by Ian Romanick
Post by Samuel Iglesias Gonsálvez
+ /* We have a source in a conversion */
+ src_alu_type =
+ nir_get_nir_type_for_glsl_type(
+ vtn_value(b, w[4], vtn_value_type_constant)-
Post by Samuel Iglesias Gonsálvez
const_type);
+ /* We use the bitsize of the conversion source to
evaluate the opcode later */
+ bit_size = glsl_get_bit_size(
+ vtn_value(b, w[4], vtn_value_type_constant)-
Post by Samuel Iglesias Gonsálvez
const_type);
+ break;
+ bit_size = glsl_get_bit_size(val->const_type);
+ };
+
+
+ nir_op op = vtn_nir_alu_op_for_spirv_opcode(opcode,
&swap, src_alu_type, dst_alu_type);
+ nir_const_value src[4];
+
for (unsigned i = 0; i < count - 4; i++) {
nir_constant *c =
vtn_value(b, w[4 + i], vtn_value_type_constant)-
Post by Samuel Iglesias Gonsálvez
constant;
Ian Romanick
2017-12-06 20:25:52 UTC
Reply
Permalink
Raw Message
Post by Samuel Iglesias Gonsálvez
Post by Ian Romanick
Post by Samuel Iglesias Gonsálvez
In that case, nir_eval_const_opcode() will evaluate the conversion
but as it was using destination's bit_size, the resulting
value was just a cast of the source constant value. By passing the
source's bit size, it does the conversion properly.
dEQP-VK.spirv_assembly.instruction.*.opspecconstantop.*convert*
---
src/compiler/spirv/spirv_to_nir.c | 31 ++++++++++++++++++++++++++-
----
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/src/compiler/spirv/spirv_to_nir.c
b/src/compiler/spirv/spirv_to_nir.c
index 2cc3c275ea9..ffbda4f1946 100644
--- a/src/compiler/spirv/spirv_to_nir.c
+++ b/src/compiler/spirv/spirv_to_nir.c
@@ -1348,14 +1348,35 @@ vtn_handle_constant(struct vtn_builder *b, SpvOp opcode,
bool swap;
nir_alu_type dst_alu_type =
nir_get_nir_type_for_glsl_type(val->const_type);
nir_alu_type src_alu_type = dst_alu_type;
- nir_op op = vtn_nir_alu_op_for_spirv_opcode(opcode,
&swap, src_alu_type, dst_alu_type);
-
unsigned num_components = glsl_get_vector_elements(val-
Post by Samuel Iglesias Gonsálvez
const_type);
- unsigned bit_size =
- glsl_get_bit_size(val->const_type);
+ unsigned bit_size;
- nir_const_value src[4];
assert(count <= 7);
+
+ switch (opcode) {
I'm not sure what we should do here. Most of these opcodes are not
valid in SpvOpSpecConstantOp in a Shader. Only SpvOpSConvert and
SpvOpFConvert are allowed. I guess it's trivial enough to support the
others anyway...
Right.
Post by Ian Romanick
do those dEQP-VK.spirv_assembly.instruction.* tests
exercise the other opcodes?
No, they only exercise SpvOpFConvert and SpvOpSConvert. Do you prefer
to just support these two opcodes for the moment?
Okay... good. I wanted to make sure they weren't expecting out-of-spec
behavior. :)
Post by Samuel Iglesias Gonsálvez
With or without that change, does it get your R-b?
Yes.
Post by Samuel Iglesias Gonsálvez
Sam
Post by Ian Romanick
Post by Samuel Iglesias Gonsálvez
+ /* We have a source in a conversion */
+ src_alu_type =
+ nir_get_nir_type_for_glsl_type(
+ vtn_value(b, w[4], vtn_value_type_constant)-
Post by Samuel Iglesias Gonsálvez
const_type);
+ /* We use the bitsize of the conversion source to
evaluate the opcode later */
+ bit_size = glsl_get_bit_size(
+ vtn_value(b, w[4], vtn_value_type_constant)-
Post by Samuel Iglesias Gonsálvez
const_type);
+ break;
+ bit_size = glsl_get_bit_size(val->const_type);
+ };
+
+
+ nir_op op = vtn_nir_alu_op_for_spirv_opcode(opcode,
&swap, src_alu_type, dst_alu_type);
+ nir_const_value src[4];
+
for (unsigned i = 0; i < count - 4; i++) {
nir_constant *c =
vtn_value(b, w[4 + i], vtn_value_type_constant)-
Post by Samuel Iglesias Gonsálvez
constant;
Loading...