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

[Ilia Mirkin]

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();

> +
>        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