[Mesa-dev] [RFC PATCH] i965: Allow C++ type safety in the use of enum brw_urb_write_flags.

Paul Berry stereotype441 at gmail.com
Fri Aug 23 14:18:31 PDT 2013


(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



More information about the mesa-dev mailing list