[Mesa-dev] [PATCH] glsl: Annotate as_foo functions that the this pointer cannot be NULL

[Ian Romanick]

On 03/18/2015 01:29 PM, Ilia Mirkin wrote:
> On Wed, Mar 18, 2015 at 4:25 PM, Ian Romanick <idr at freedesktop.org> wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> We use the idoim
>>
>>    ir_foo *x = y->as_foo();
>>    if (x == NULL)
>>       return;
>>
>> all over the place.  GCC generates some quite lovely code for this.
>> One such example:
>>
>>   340a5b:       83 7d 18 04             cmpl   $0x4,0x18(%rbp)
>>   340a5f:       0f 85 06 04 00 00       jne    340e6b
>>   340a65:       48 85 ed                test   %rbp,%rbp
>>   340a68:       0f 84 fd 03 00 00       je     340e6b
>>
>> This case used as_expression() (ir_type_expression is 4).  Note that it
>> checkes the ir_type, then checks that the pointer isn't NULL.  There is
>> some disconnect in GCC around the condition in the as_foo functions.
>>
>>       return ir_type == ir_type_##TYPE ? (ir_##TYPE *) this : NULL; \
>>
>> It believes "this" could be NULL, so it emits check outside the function
>> just for fun.
>>
>> This patch uses unreachable() to tell GCC that it need not bother with
>> extra NULL checking of pointer returned by the as_foo functions.
>>
>>    text    data     bss     dec     hex filename
>> 4836430  158688   26248 5021366  4c9eb6 i965_dri-before.so
>> 4836173  158688   26248 5021109  4c9db5 i965_dri-after.so
>>
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> ---
>>  src/glsl/ir.h | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/src/glsl/ir.h b/src/glsl/ir.h
>> index 25f2eca..89887ba 100644
>> --- a/src/glsl/ir.h
>> +++ b/src/glsl/ir.h
>> @@ -119,6 +119,9 @@ public:
>>     /*@{*/
>>     class ir_rvalue *as_rvalue()
>>     {
>> +      if (this == NULL)
>> +         unreachable("this cannot be NULL");
> 
> Would this be a good use-case for "assume"? i.e. assume(this). IMHO
> that reads better than if (cond) unreachable();

Yes... that actually seems kind of obvious now. :(  I tried that, and it
produces identical code.  Thanks. :)

>> +
>>        if (ir_type == ir_type_dereference_array ||
>>            ir_type == ir_type_dereference_record ||
>>            ir_type == ir_type_dereference_variable ||
>> @@ -132,6 +135,9 @@ public:
>>
>>     class ir_dereference *as_dereference()
>>     {
>> +      if (this == NULL)
>> +         unreachable("this cannot be NULL");
>> +
>>        if (ir_type == ir_type_dereference_array ||
>>            ir_type == ir_type_dereference_record ||
>>            ir_type == ir_type_dereference_variable)
>> @@ -141,6 +147,9 @@ public:
>>
>>     class ir_jump *as_jump()
>>     {
>> +      if (this == NULL)
>> +         unreachable("this cannot be NULL");
>> +
>>        if (ir_type == ir_type_loop_jump ||
>>            ir_type == ir_type_return ||
>>            ir_type == ir_type_discard)
>> @@ -151,6 +160,7 @@ public:
>>     #define AS_CHILD(TYPE) \
>>     class ir_##TYPE * as_##TYPE() \
>>     { \
>> +      if (this == NULL) unreachable("this cannot be NULL");         \
>>        return ir_type == ir_type_##TYPE ? (ir_##TYPE *) this : NULL; \
>>     }
>>     AS_CHILD(variable)
>> --
>> 2.1.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>