Re: [Mesa-dev] [PATCH 02/13] nir/spirv: Use unreachable("...") rather than assert(!"...")

2017-08-21 Thread Jason Ekstrand 
On Mon, Aug 21, 2017 at 2:44 PM, Matt Turner  wrote:

> On Thu, Aug 10, 2017 at 7:02 PM, Jordan Justen
>  wrote:
> > On 2017-08-10 15:02:33, Matt Turner wrote:
> >> Quiets a number of uninitialized variable warnings in clang.
> >> ---
> >>  src/compiler/spirv/spirv_to_nir.c  | 24 
> >>  src/compiler/spirv/vtn_variables.c | 10 +-
> >>  2 files changed, 17 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/src/compiler/spirv/spirv_to_nir.c
> b/src/compiler/spirv/spirv_to_nir.c
> >> index 7b34dad30c..870dda0314 100644
> >> --- a/src/compiler/spirv/spirv_to_nir.c
> >> +++ b/src/compiler/spirv/spirv_to_nir.c
> >> @@ -262,7 +262,7 @@ vtn_handle_extension(struct vtn_builder *b, SpvOp
> opcode,
> >>if (strcmp((const char *)&w[2], "GLSL.std.450") == 0) {
> >>   val->ext_handler = vtn_handle_glsl450_instruction;
> >>} else {
> >> - assert(!"Unsupported extension");
> >> + unreachable("Unsupported extension");
> >>}
> >>break;
> >> }
> >> @@ -724,7 +724,7 @@ translate_image_format(SpvImageFormat format)
> >> case SpvImageFormatR16ui:return 0x8234; /* GL_R16UI */
> >> case SpvImageFormatR8ui: return 0x8232; /* GL_R8UI */
> >> default:
> >> -  assert(!"Invalid image format");gl_primitive_from_
> spv_execution_mode
> >> +  unreachable("Invalid image format");
> >>return 0;
> >> }
> >>  }
> >> @@ -785,7 +785,7 @@ vtn_handle_type(struct vtn_builder *b, SpvOp opcode,
> >>val->type->type = glsl_matrix_type(glsl_get_
> base_type(base->type),
> >>   glsl_get_vector_elements(base-
> >type),
> >>   columns);
> >> -  assert(!glsl_type_is_error(val->type->type));
> >> +  unreachable(glsl_type_is_error(val->type->type));
> >
> > I think we want assert here, right?
>
> Yes. How sloppy. Thanks.
>
> I was hoping I could get a comment from Jason. There seem to be vague
> attempts to keep things going even after an assert (return 4 in
> gl_primitive_from_spv_execution_mode for instance). Are you okay with
> making these unreachables? If so, I'll respin the patch and drop that
> now-dead code as well as fixing the still-should-be-asserts noted by
> Jordan.
>

I'm fine with it.  I have some patches on the list to add a new vtn_fail
function which longjumps and then cleans up in which case, we'll replace
the unreachable() with vtn_fail().  However, it's marked __noreturn__ so
clang should also know that it's not a fall-through in that case as well.

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

Re: [Mesa-dev] [PATCH 02/13] nir/spirv: Use unreachable("...") rather than assert(!"...")

2017-08-21 Thread Matt Turner 
On Thu, Aug 10, 2017 at 7:02 PM, Jordan Justen
 wrote:
> On 2017-08-10 15:02:33, Matt Turner wrote:
>> Quiets a number of uninitialized variable warnings in clang.
>> ---
>>  src/compiler/spirv/spirv_to_nir.c  | 24 
>>  src/compiler/spirv/vtn_variables.c | 10 +-
>>  2 files changed, 17 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/compiler/spirv/spirv_to_nir.c 
>> b/src/compiler/spirv/spirv_to_nir.c
>> index 7b34dad30c..870dda0314 100644
>> --- a/src/compiler/spirv/spirv_to_nir.c
>> +++ b/src/compiler/spirv/spirv_to_nir.c
>> @@ -262,7 +262,7 @@ vtn_handle_extension(struct vtn_builder *b, SpvOp opcode,
>>if (strcmp((const char *)&w[2], "GLSL.std.450") == 0) {
>>   val->ext_handler = vtn_handle_glsl450_instruction;
>>} else {
>> - assert(!"Unsupported extension");
>> + unreachable("Unsupported extension");
>>}
>>break;
>> }
>> @@ -724,7 +724,7 @@ translate_image_format(SpvImageFormat format)
>> case SpvImageFormatR16ui:return 0x8234; /* GL_R16UI */
>> case SpvImageFormatR8ui: return 0x8232; /* GL_R8UI */
>> default:
>> -  assert(!"Invalid image format");gl_primitive_from_spv_execution_mode
>> +  unreachable("Invalid image format");
>>return 0;
>> }
>>  }
>> @@ -785,7 +785,7 @@ vtn_handle_type(struct vtn_builder *b, SpvOp opcode,
>>val->type->type = glsl_matrix_type(glsl_get_base_type(base->type),
>>   
>> glsl_get_vector_elements(base->type),
>>   columns);
>> -  assert(!glsl_type_is_error(val->type->type));
>> +  unreachable(glsl_type_is_error(val->type->type));
>
> I think we want assert here, right?

Yes. How sloppy. Thanks.

I was hoping I could get a comment from Jason. There seem to be vague
attempts to keep things going even after an assert (return 4 in
gl_primitive_from_spv_execution_mode for instance). Are you okay with
making these unreachables? If so, I'll respin the patch and drop that
now-dead code as well as fixing the still-should-be-asserts noted by
Jordan.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 02/13] nir/spirv: Use unreachable("...") rather than assert(!"...")

2017-08-10 Thread Jordan Justen 
On 2017-08-10 15:02:33, Matt Turner wrote:
> Quiets a number of uninitialized variable warnings in clang.
> ---
>  src/compiler/spirv/spirv_to_nir.c  | 24 
>  src/compiler/spirv/vtn_variables.c | 10 +-
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/src/compiler/spirv/spirv_to_nir.c 
> b/src/compiler/spirv/spirv_to_nir.c
> index 7b34dad30c..870dda0314 100644
> --- a/src/compiler/spirv/spirv_to_nir.c
> +++ b/src/compiler/spirv/spirv_to_nir.c
> @@ -262,7 +262,7 @@ vtn_handle_extension(struct vtn_builder *b, SpvOp opcode,
>if (strcmp((const char *)&w[2], "GLSL.std.450") == 0) {
>   val->ext_handler = vtn_handle_glsl450_instruction;
>} else {
> - assert(!"Unsupported extension");
> + unreachable("Unsupported extension");
>}
>break;
> }
> @@ -724,7 +724,7 @@ translate_image_format(SpvImageFormat format)
> case SpvImageFormatR16ui:return 0x8234; /* GL_R16UI */
> case SpvImageFormatR8ui: return 0x8232; /* GL_R8UI */
> default:
> -  assert(!"Invalid image format");
> +  unreachable("Invalid image format");
>return 0;
> }
>  }
> @@ -785,7 +785,7 @@ vtn_handle_type(struct vtn_builder *b, SpvOp opcode,
>val->type->type = glsl_matrix_type(glsl_get_base_type(base->type),
>   
> glsl_get_vector_elements(base->type),
>   columns);
> -  assert(!glsl_type_is_error(val->type->type));
> +  unreachable(glsl_type_is_error(val->type->type));

I think we want assert here, right?

>val->type->length = columns;
>val->type->array_element = base;
>val->type->row_major = false;
> @@ -919,7 +919,7 @@ vtn_handle_type(struct vtn_builder *b, SpvOp opcode,
>   else if (dim == GLSL_SAMPLER_DIM_SUBPASS)
>  dim = GLSL_SAMPLER_DIM_SUBPASS_MS;
>   else
> -assert(!"Unsupported multisampled image type");
> +unreachable("Unsupported multisampled image type");
>}
>  
>val->type->image_format = translate_image_format(format);
> @@ -929,12 +929,12 @@ vtn_handle_type(struct vtn_builder *b, SpvOp opcode,
>   val->type->type = glsl_sampler_type(dim, is_shadow, is_array,
>   
> glsl_get_base_type(sampled_type));
>} else if (sampled == 2) {
> - assert(!is_shadow);
> + unreachable(is_shadow);

and here ...

>   val->type->sampled = false;
>   val->type->type = glsl_image_type(dim, is_array,
> glsl_get_base_type(sampled_type));
>} else {
> - assert(!"We need to know if the image will be sampled");
> + unreachable("We need to know if the image will be sampled");
>}
>break;
> }
> @@ -1378,7 +1378,7 @@ vtn_handle_constant(struct vtn_builder *b, SpvOp opcode,
>break;
>  
> case SpvOpConstantSampler:
> -  assert(!"OpConstantSampler requires Kernel Capability");
> +  unreachable("OpConstantSampler requires Kernel Capability");
>break;
>  
> default:
> @@ -2626,7 +2626,7 @@ gl_primitive_from_spv_execution_mode(SpvExecutionMode 
> mode)
> case SpvExecutionModeOutputTriangleStrip:
>return 5; /* GL_TRIANGLE_STRIP */
> default:
> -  assert(!"Invalid primitive type");
> +  unreachable("Invalid primitive type");
>return 4;
> }
>  }
> @@ -2646,7 +2646,7 @@ vertices_in_from_spv_execution_mode(SpvExecutionMode 
> mode)
> case SpvExecutionModeInputTrianglesAdjacency:
>return 6;
> default:
> -  assert(!"Invalid GS input mode");
> +  unreachable("Invalid GS input mode");
>return 0;
> }
>  }
> @@ -2974,7 +2974,7 @@ vtn_handle_execution_mode(struct vtn_builder *b, struct 
> vtn_value *entry_point,
>break;
>  
> case SpvExecutionModeXfb:
> -  assert(!"Unhandled execution mode");
> +  unreachable("Unhandled execution mode");
>break;
>  
> case SpvExecutionModeVecTypeHint:
> @@ -3008,7 +3008,7 @@ vtn_handle_variable_or_type_instruction(struct 
> vtn_builder *b, SpvOp opcode,
> case SpvOpMemberDecorate:
> case SpvOpGroupDecorate:
> case SpvOpGroupMemberDecorate:
> -  assert(!"Invalid opcode types and variables section");
> +  unreachable("Invalid opcode types and variables section");
>break;
>  
> case SpvOpTypeVoid:
> @@ -3348,7 +3348,7 @@ spirv_to_nir(const uint32_t *words, size_t word_count,
> vtn_handle_preamble_instruction);
>  
> if (b->entry_point == NULL) {
> -  assert(!"Entry point not found");
> +  unreachable("Entry point not found");
>ralloc_free(b);
>return NULL;
> }
> diff --git a/src/compiler/spirv/vtn_variables.c 
> b/src/compiler/spirv/vtn_variables.c
> index 4432e72e54..077ad7bb1c 100644
> --- a/src/compiler/s

[Mesa-dev] [PATCH 02/13] nir/spirv: Use unreachable("...") rather than assert(!"...")

2017-08-10 Thread Matt Turner 
Quiets a number of uninitialized variable warnings in clang.
---
 src/compiler/spirv/spirv_to_nir.c  | 24 
 src/compiler/spirv/vtn_variables.c | 10 +-
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/src/compiler/spirv/spirv_to_nir.c 
b/src/compiler/spirv/spirv_to_nir.c
index 7b34dad30c..870dda0314 100644
--- a/src/compiler/spirv/spirv_to_nir.c
+++ b/src/compiler/spirv/spirv_to_nir.c
@@ -262,7 +262,7 @@ vtn_handle_extension(struct vtn_builder *b, SpvOp opcode,
   if (strcmp((const char *)&w[2], "GLSL.std.450") == 0) {
  val->ext_handler = vtn_handle_glsl450_instruction;
   } else {
- assert(!"Unsupported extension");
+ unreachable("Unsupported extension");
   }
   break;
}
@@ -724,7 +724,7 @@ translate_image_format(SpvImageFormat format)
case SpvImageFormatR16ui:return 0x8234; /* GL_R16UI */
case SpvImageFormatR8ui: return 0x8232; /* GL_R8UI */
default:
-  assert(!"Invalid image format");
+  unreachable("Invalid image format");
   return 0;
}
 }
@@ -785,7 +785,7 @@ vtn_handle_type(struct vtn_builder *b, SpvOp opcode,
   val->type->type = glsl_matrix_type(glsl_get_base_type(base->type),
  glsl_get_vector_elements(base->type),
  columns);
-  assert(!glsl_type_is_error(val->type->type));
+  unreachable(glsl_type_is_error(val->type->type));
   val->type->length = columns;
   val->type->array_element = base;
   val->type->row_major = false;
@@ -919,7 +919,7 @@ vtn_handle_type(struct vtn_builder *b, SpvOp opcode,
  else if (dim == GLSL_SAMPLER_DIM_SUBPASS)
 dim = GLSL_SAMPLER_DIM_SUBPASS_MS;
  else
-assert(!"Unsupported multisampled image type");
+unreachable("Unsupported multisampled image type");
   }
 
   val->type->image_format = translate_image_format(format);
@@ -929,12 +929,12 @@ vtn_handle_type(struct vtn_builder *b, SpvOp opcode,
  val->type->type = glsl_sampler_type(dim, is_shadow, is_array,
  glsl_get_base_type(sampled_type));
   } else if (sampled == 2) {
- assert(!is_shadow);
+ unreachable(is_shadow);
  val->type->sampled = false;
  val->type->type = glsl_image_type(dim, is_array,
glsl_get_base_type(sampled_type));
   } else {
- assert(!"We need to know if the image will be sampled");
+ unreachable("We need to know if the image will be sampled");
   }
   break;
}
@@ -1378,7 +1378,7 @@ vtn_handle_constant(struct vtn_builder *b, SpvOp opcode,
   break;
 
case SpvOpConstantSampler:
-  assert(!"OpConstantSampler requires Kernel Capability");
+  unreachable("OpConstantSampler requires Kernel Capability");
   break;
 
default:
@@ -2626,7 +2626,7 @@ gl_primitive_from_spv_execution_mode(SpvExecutionMode 
mode)
case SpvExecutionModeOutputTriangleStrip:
   return 5; /* GL_TRIANGLE_STRIP */
default:
-  assert(!"Invalid primitive type");
+  unreachable("Invalid primitive type");
   return 4;
}
 }
@@ -2646,7 +2646,7 @@ vertices_in_from_spv_execution_mode(SpvExecutionMode mode)
case SpvExecutionModeInputTrianglesAdjacency:
   return 6;
default:
-  assert(!"Invalid GS input mode");
+  unreachable("Invalid GS input mode");
   return 0;
}
 }
@@ -2974,7 +2974,7 @@ vtn_handle_execution_mode(struct vtn_builder *b, struct 
vtn_value *entry_point,
   break;
 
case SpvExecutionModeXfb:
-  assert(!"Unhandled execution mode");
+  unreachable("Unhandled execution mode");
   break;
 
case SpvExecutionModeVecTypeHint:
@@ -3008,7 +3008,7 @@ vtn_handle_variable_or_type_instruction(struct 
vtn_builder *b, SpvOp opcode,
case SpvOpMemberDecorate:
case SpvOpGroupDecorate:
case SpvOpGroupMemberDecorate:
-  assert(!"Invalid opcode types and variables section");
+  unreachable("Invalid opcode types and variables section");
   break;
 
case SpvOpTypeVoid:
@@ -3348,7 +3348,7 @@ spirv_to_nir(const uint32_t *words, size_t word_count,
vtn_handle_preamble_instruction);
 
if (b->entry_point == NULL) {
-  assert(!"Entry point not found");
+  unreachable("Entry point not found");
   ralloc_free(b);
   return NULL;
}
diff --git a/src/compiler/spirv/vtn_variables.c 
b/src/compiler/spirv/vtn_variables.c
index 4432e72e54..077ad7bb1c 100644
--- a/src/compiler/spirv/vtn_variables.c
+++ b/src/compiler/spirv/vtn_variables.c
@@ -172,7 +172,7 @@ vtn_ssa_offset_pointer_dereference(struct vtn_builder *b,
   }
 
   /* This is the first access chain so we also need an offset */
-  assert(!offset);
+  unreachable(offset);
   offset = nir_imm_int(&b->nb, 0);
}
assert(offset);
@@ -599,7 +5