[Mesa-dev] [RFC PATCH] i965: Allow C++ type safety in the use of enum brw_urb_write_flags.
Mark Mueller
markkmueller at gmail.com
Fri Aug 23 14:36:15 PDT 2013
This is a nice improvement over the explicit cast, which is how I've always
done it in the past - which is the ugly part of an otherwise great method
for flags. Also I use & a lot with enum for clearing bits.
On Fri, Aug 23, 2013 at 3:18 PM, Paul Berry <stereotype441 at gmail.com> wrote:
> (From a suggestion by Francisco Jerez)
>
> If an enum represents a bitfield of flags, e.g.:
>
> enum E {
> A = 1,
> B = 2,
> C = 4,
> D = 8,
> };
>
> then C++ normally prohibits statements like this:
>
> enum E x = A | B;
>
> because A and B are implicitly converted to ints before OR-ing them,
> and an int can't be stored in an enum without a type cast. C, on the
> other hand, allows an int to be implicitly converted to an enum
> without casting.
>
> In the past we've dealt with this situation by storing flag bitfields
> as ints. This avoids ugly casting at the expense of some type safety
> that C++ would normally have offered (e.g. we get no warning if we
> accidentally use the wrong enum type).
>
> However, we can get the best of both worlds if we override the |
> operator. The ugly casting is confined to the operator overload, and
> we still get the benefit of C++ making sure we don't use the wrong
> enum type.
>
> The disadvantages are that (a) we need an explicit enum value for 0,
> and (b) we can't use related operators like |= unless we define
> additional overloads.
>
> So what do folks think? Is it worth it?
>
> Cc: Francisco Jerez <currojerez at riseup.net>
> ---
> src/mesa/drivers/dri/i965/brw_clip.h | 2 +-
> src/mesa/drivers/dri/i965/brw_clip_util.c | 2 +-
> src/mesa/drivers/dri/i965/brw_eu.h | 16 +++++++++++++++-
> src/mesa/drivers/dri/i965/brw_eu_emit.c | 4 ++--
> src/mesa/drivers/dri/i965/brw_sf_emit.c | 12 ++++++++----
> src/mesa/drivers/dri/i965/brw_vec4.h | 2 +-
> src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 3 ++-
> 7 files changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_clip.h
> b/src/mesa/drivers/dri/i965/brw_clip.h
> index 5af0ad3..41f5c75 100644
> --- a/src/mesa/drivers/dri/i965/brw_clip.h
> +++ b/src/mesa/drivers/dri/i965/brw_clip.h
> @@ -173,7 +173,7 @@ void brw_clip_init_planes( struct brw_clip_compile *c
> );
>
> void brw_clip_emit_vue(struct brw_clip_compile *c,
> struct brw_indirect vert,
> - unsigned flags,
> + enum brw_urb_write_flags flags,
> GLuint header);
>
> void brw_clip_kill_thread(struct brw_clip_compile *c);
> diff --git a/src/mesa/drivers/dri/i965/brw_clip_util.c
> b/src/mesa/drivers/dri/i965/brw_clip_util.c
> index d5c50d7..24d053e 100644
> --- a/src/mesa/drivers/dri/i965/brw_clip_util.c
> +++ b/src/mesa/drivers/dri/i965/brw_clip_util.c
> @@ -313,7 +313,7 @@ void brw_clip_interp_vertex( struct brw_clip_compile
> *c,
>
> void brw_clip_emit_vue(struct brw_clip_compile *c,
> struct brw_indirect vert,
> - unsigned flags,
> + enum brw_urb_write_flags flags,
> GLuint header)
> {
> struct brw_compile *p = &c->func;
> diff --git a/src/mesa/drivers/dri/i965/brw_eu.h
> b/src/mesa/drivers/dri/i965/brw_eu.h
> index 9053ea2..069b223 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu.h
> +++ b/src/mesa/drivers/dri/i965/brw_eu.h
> @@ -229,6 +229,8 @@ void brw_set_dp_write_message(struct brw_compile *p,
> GLuint send_commit_msg);
>
> enum brw_urb_write_flags {
> + BRW_URB_WRITE_NO_FLAGS = 0,
> +
> /**
> * Causes a new URB entry to be allocated, and its address stored in
> the
> * destination register (gen < 7).
> @@ -271,11 +273,23 @@ enum brw_urb_write_flags {
> BRW_URB_WRITE_ALLOCATE | BRW_URB_WRITE_COMPLETE,
> };
>
> +#ifdef __cplusplus
> +/**
> + * Allow brw_urb_write_flags enums to be ORed together (normally C++
> wouldn't
> + * allow this without a type cast).
> + */
> +inline enum brw_urb_write_flags
> +operator|(enum brw_urb_write_flags x, enum brw_urb_write_flags y)
> +{
> + return (enum brw_urb_write_flags) ((int) x | (int) y);
> +}
> +#endif
> +
> void brw_urb_WRITE(struct brw_compile *p,
> struct brw_reg dest,
> GLuint msg_reg_nr,
> struct brw_reg src0,
> - unsigned flags,
> + enum brw_urb_write_flags flags,
> GLuint msg_length,
> GLuint response_length,
> GLuint offset,
> diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> index b55b57e..ecf8597 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> @@ -515,7 +515,7 @@ static void brw_set_ff_sync_message(struct brw_compile
> *p,
>
> static void brw_set_urb_message( struct brw_compile *p,
> struct brw_instruction *insn,
> - unsigned flags,
> + enum brw_urb_write_flags flags,
> GLuint msg_length,
> GLuint response_length,
> GLuint offset,
> @@ -2213,7 +2213,7 @@ void brw_urb_WRITE(struct brw_compile *p,
> struct brw_reg dest,
> GLuint msg_reg_nr,
> struct brw_reg src0,
> - unsigned flags,
> + enum brw_urb_write_flags flags,
> GLuint msg_length,
> GLuint response_length,
> GLuint offset,
> diff --git a/src/mesa/drivers/dri/i965/brw_sf_emit.c
> b/src/mesa/drivers/dri/i965/brw_sf_emit.c
> index d329bef..b206797 100644
> --- a/src/mesa/drivers/dri/i965/brw_sf_emit.c
> +++ b/src/mesa/drivers/dri/i965/brw_sf_emit.c
> @@ -491,7 +491,8 @@ void brw_emit_tri_setup(struct brw_sf_compile *c, bool
> allocate)
> brw_null_reg(),
> 0,
> brw_vec8_grf(0, 0), /* r0, will be copied to m0 */
> - last ? BRW_URB_WRITE_EOT_COMPLETE : 0,
> + last ? BRW_URB_WRITE_EOT_COMPLETE
> + : BRW_URB_WRITE_NO_FLAGS,
> 4, /* msg len */
> 0, /* response len */
> i*4, /* offset */
> @@ -562,7 +563,8 @@ void brw_emit_line_setup(struct brw_sf_compile *c,
> bool allocate)
> brw_null_reg(),
> 0,
> brw_vec8_grf(0, 0),
> - last ? BRW_URB_WRITE_EOT_COMPLETE : 0,
> + last ? BRW_URB_WRITE_EOT_COMPLETE
> + : BRW_URB_WRITE_NO_FLAGS,
> 4, /* msg len */
> 0, /* response len */
> i*4, /* urb destination offset */
> @@ -649,7 +651,8 @@ void brw_emit_point_sprite_setup(struct brw_sf_compile
> *c, bool allocate)
> brw_null_reg(),
> 0,
> brw_vec8_grf(0, 0),
> - last ? BRW_URB_WRITE_EOT_COMPLETE : 0,
> + last ? BRW_URB_WRITE_EOT_COMPLETE
> + : BRW_URB_WRITE_NO_FLAGS,
> 4, /* msg len */
> 0, /* response len */
> i*4, /* urb destination offset */
> @@ -706,7 +709,8 @@ void brw_emit_point_setup(struct brw_sf_compile *c,
> bool allocate)
> brw_null_reg(),
> 0,
> brw_vec8_grf(0, 0),
> - last ? BRW_URB_WRITE_EOT_COMPLETE : 0,
> + last ? BRW_URB_WRITE_EOT_COMPLETE
> + : BRW_URB_WRITE_NO_FLAGS,
> 4, /* msg len */
> 0, /* response len */
> i*4, /* urb destination offset */
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h
> b/src/mesa/drivers/dri/i965/brw_vec4.h
> index 5d8f0bf..0cb0347 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.h
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
> @@ -222,7 +222,7 @@ public:
> int target; /**< MRT target. */
> bool shadow_compare;
>
> - unsigned urb_write_flags;
> + enum brw_urb_write_flags urb_write_flags;
> bool header_present;
> int mlen; /**< SEND message length */
> int base_mrf; /**< First MRF in the SEND message, if mlen is nonzero.
> */
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> index 72841e7..d0959b7 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> @@ -2788,7 +2788,8 @@ vec4_vs_visitor::emit_urb_write_opcode(bool complete)
> }
>
> vec4_instruction *inst = emit(VS_OPCODE_URB_WRITE);
> - inst->urb_write_flags = complete ? BRW_URB_WRITE_EOT_COMPLETE : 0;
> + inst->urb_write_flags = complete ?
> + BRW_URB_WRITE_EOT_COMPLETE : BRW_URB_WRITE_NO_FLAGS;
>
> return inst;
> }
> --
> 1.8.3.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130823/65025214/attachment.html>
More information about the mesa-dev
mailing list