diff mbox

[v2,14/26] tcg: rework TCG helper flags

Message ID 1349812584-19551-15-git-send-email-aurelien@aurel32.net
State New
Headers show

Commit Message

Aurelien Jarno Oct. 9, 2012, 7:56 p.m. UTC 
The current helper flags, TCG_CALL_CONST and TCG_CALL_PURE might be
confusing and doesn't provide enough granularity for some helpers (FP
helpers for example).

This patch changes them into the following helpers flags:
- TCG_CALL_NO_READ_GLOBALS means that the helper does not read globals,
  either directly or via an exception. They will not be saved to their
  canonical location before calling the helper.
- TCG_CALL_NO_WRITE_GLOBALS means that the helper does not modify any
  globals. They will only be saved to their canonical locations before
  calling helpers, but they won't be reloaded afterwise.
- TCG_CALL_NO_SIDE_EFFECTS means that the call to the function is
  removed if the return value is not used.

It provides convenience flags, to avoid helper definitions longer than
80 characters. It also provides compatibility flags, and updates the
documentation.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 tcg/README     |   19 ++++++++++++++-----
 tcg/optimize.c |    3 ++-
 tcg/tcg-op.h   |   18 ++++++++++--------
 tcg/tcg.c      |   24 ++++++++++++++++--------
 tcg/tcg.h      |   26 ++++++++++++++++++--------
 5 files changed, 60 insertions(+), 30 deletions(-)

Comments

Richard Henderson Oct. 10, 2012, 5:11 p.m. UTC | #1 
On 10/09/2012 12:56 PM, Aurelien Jarno wrote:
> +                    if (!(call_flags & (TCG_CALL_NO_WRITE_GLOBALS |
> +                                        TCG_CALL_NO_READ_GLOBALS))) {

Code like this would be shorter, and perhaps clearer, by

> +/* Helper does not read globals (either directly or through an exception). It
> +   implies TCG_CALL_NO_WRITE_GLOBALS. */
> +#define TCG_CALL_NO_READ_GLOBALS    0x0010
> +/* Helper does not write globals */
> +#define TCG_CALL_NO_WRITE_GLOBALS   0x0020

having RG actually include WG, i.e.

#define TCG_CALL_NO_READ_GLOBALS  0x0030

That said, 

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~
Richard Henderson Oct. 10, 2012, 5:12 p.m. UTC | #2 
On 10/09/2012 01:24 PM, Aurelien Jarno wrote:
> Maybe NO_RG_SE?

Perhaps, yes.  But I certainly can't think of anything better.


r~
Richard Henderson Oct. 10, 2012, 5:14 p.m. UTC | #3 
On 10/10/2012 10:12 AM, Richard Henderson wrote:
> On 10/09/2012 01:24 PM, Aurelien Jarno wrote:
>> Maybe NO_RG_SE?
> 
> Perhaps, yes.  But I certainly can't think of anything better.

How about NO_RWG_SE.  I like having the fact that R implies W
be included in the symbol.


r~
Aurelien Jarno Oct. 19, 2012, 8:24 p.m. UTC | #4 
On Wed, Oct 10, 2012 at 10:11:54AM -0700, Richard Henderson wrote:
> On 10/09/2012 12:56 PM, Aurelien Jarno wrote:
> > +                    if (!(call_flags & (TCG_CALL_NO_WRITE_GLOBALS |
> > +                                        TCG_CALL_NO_READ_GLOBALS))) {
> 
> Code like this would be shorter, and perhaps clearer, by
> 
> > +/* Helper does not read globals (either directly or through an exception). It
> > +   implies TCG_CALL_NO_WRITE_GLOBALS. */
> > +#define TCG_CALL_NO_READ_GLOBALS    0x0010
> > +/* Helper does not write globals */
> > +#define TCG_CALL_NO_WRITE_GLOBALS   0x0020
> 
> having RG actually include WG, i.e.
> 
> #define TCG_CALL_NO_READ_GLOBALS  0x0030
> 

Agreed. On the other hand, the few lines just above the one your quoted
would be more complicated:

+                    if (!(call_flags & TCG_CALL_NO_READ_GLOBALS)) {
+                        /* globals should be synced to memory */
+                        memset(mem_temps, 1, s->nb_globals);
+                    }

would have to be written:

+                    if (!(call_flags & TCG_CALL_NO_READ_WRITE_GLOBALS) ||
+                        (call_flags & TCG_CALL_WRITE_GLOBALS)) {
+                        /* globals should be synced to memory */
+                        memset(mem_temps, 1, s->nb_globals);
+                    }

Note sure it makes the things clearer, and definitely not shorter.
Richard Henderson Oct. 19, 2012, 10:25 p.m. UTC | #5 
On 2012-10-20 06:24, Aurelien Jarno wrote:
> Agreed. On the other hand, the few lines just above the one your quoted
> would be more complicated:
> 
> +                    if (!(call_flags & TCG_CALL_NO_READ_GLOBALS)) {
> +                        /* globals should be synced to memory */
> +                        memset(mem_temps, 1, s->nb_globals);
> +                    }
> 
> would have to be written:
> 
> +                    if (!(call_flags & TCG_CALL_NO_READ_WRITE_GLOBALS) ||
> +                        (call_flags & TCG_CALL_WRITE_GLOBALS)) {
> +                        /* globals should be synced to memory */
> +                        memset(mem_temps, 1, s->nb_globals);
> +                    }
> 
> Note sure it makes the things clearer, and definitely not shorter.

Ah, didn't think of that.  Oh well.


r~
diff mbox

Patch

diff --git a/tcg/README b/tcg/README
index 9d1b100..ec1ac79 100644
--- a/tcg/README
+++ b/tcg/README
@@ -77,11 +77,20 @@  destroyed, but local temporaries and globals are preserved.
 Using the tcg_gen_helper_x_y it is possible to call any function
 taking i32, i64 or pointer types. By default, before calling a helper,
 all globals are stored at their canonical location and it is assumed
-that the function can modify them. This can be overridden by the
-TCG_CALL_CONST function modifier. By default, the helper is allowed to
-modify the CPU state or raise an exception. This can be overridden by
-the TCG_CALL_PURE function modifier, in which case the call to the
-function is removed if the return value is not used.
+that the function can modify them. By default, the helper is allowed to
+modify the CPU state or raise an exception.
+
+This can be overridden using the following function modifiers:
+- TCG_CALL_NO_READ_GLOBALS means that the helper does not read globals,
+  either directly or via an exception. They will not be saved to their
+  canonical locations before calling the helper.
+- TCG_CALL_NO_WRITE_GLOBALS means that the helper does not modify any globals.
+  They will only be saved to their canonical location before calling helpers,
+  but they won't be reloaded afterwise.
+- TCG_CALL_NO_SIDE_EFFECTS means that the call to the function is removed if
+  the return value is not used.
+
+Note that TCG_CALL_NO_READ_GLOBALS implies TCG_CALL_NO_WRITE_GLOBALS.
 
 On some TCG targets (e.g. x86), several calling conventions are
 supported.
diff --git a/tcg/optimize.c b/tcg/optimize.c
index edb2b0e..720fb9d 100644
--- a/tcg/optimize.c
+++ b/tcg/optimize.c
@@ -760,7 +760,8 @@  static TCGArg *tcg_constant_folding(TCGContext *s, uint16_t *tcg_opc_ptr,
             break;
         case INDEX_op_call:
             nb_call_args = (args[0] >> 16) + (args[0] & 0xffff);
-            if (!(args[nb_call_args + 1] & (TCG_CALL_CONST | TCG_CALL_PURE))) {
+            if (!(args[nb_call_args + 1] & (TCG_CALL_NO_READ_GLOBALS |
+                                            TCG_CALL_NO_WRITE_GLOBALS))) {
                 for (i = 0; i < nb_globals; i++) {
                     reset_temp(i);
                 }
diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h
index 5518458..f07e740 100644
--- a/tcg/tcg-op.h
+++ b/tcg/tcg-op.h
@@ -396,10 +396,10 @@  static inline void tcg_gen_helperN(void *func, int flags, int sizemask,
 }
 
 /* Note: Both tcg_gen_helper32() and tcg_gen_helper64() are currently
-   reserved for helpers in tcg-runtime.c. These helpers are all const
-   and pure, hence the call to tcg_gen_callN() with TCG_CALL_CONST |
-   TCG_CALL_PURE. This may need to be adjusted if these functions
-   start to be used with other helpers. */
+   reserved for helpers in tcg-runtime.c. These helpers all do not read
+   globals and do not have side effects, hence the call to tcg_gen_callN()
+   with TCG_CALL_NO_READ_GLOBALS | TCG_CALL_NO_SIDE_EFFECTS. This may need
+   to be adjusted if these functions start to be used with other helpers. */
 static inline void tcg_gen_helper32(void *func, int sizemask, TCGv_i32 ret,
                                     TCGv_i32 a, TCGv_i32 b)
 {
@@ -408,8 +408,9 @@  static inline void tcg_gen_helper32(void *func, int sizemask, TCGv_i32 ret,
     fn = tcg_const_ptr(func);
     args[0] = GET_TCGV_I32(a);
     args[1] = GET_TCGV_I32(b);
-    tcg_gen_callN(&tcg_ctx, fn, TCG_CALL_CONST | TCG_CALL_PURE, sizemask,
-                  GET_TCGV_I32(ret), 2, args);
+    tcg_gen_callN(&tcg_ctx, fn,
+                  TCG_CALL_NO_READ_GLOBALS | TCG_CALL_NO_SIDE_EFFECTS,
+                  sizemask, GET_TCGV_I32(ret), 2, args);
     tcg_temp_free_ptr(fn);
 }
 
@@ -421,8 +422,9 @@  static inline void tcg_gen_helper64(void *func, int sizemask, TCGv_i64 ret,
     fn = tcg_const_ptr(func);
     args[0] = GET_TCGV_I64(a);
     args[1] = GET_TCGV_I64(b);
-    tcg_gen_callN(&tcg_ctx, fn, TCG_CALL_CONST | TCG_CALL_PURE, sizemask,
-                  GET_TCGV_I64(ret), 2, args);
+    tcg_gen_callN(&tcg_ctx, fn,
+                  TCG_CALL_NO_READ_GLOBALS | TCG_CALL_NO_SIDE_EFFECTS,
+                  sizemask, GET_TCGV_I64(ret), 2, args);
     tcg_temp_free_ptr(fn);
 }
 
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 3f30fb1..131cdd4 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -1255,7 +1255,7 @@  static void tcg_liveness_analysis(TCGContext *s)
 
                 /* pure functions can be removed if their result is not
                    used */
-                if (call_flags & TCG_CALL_PURE) {
+                if (call_flags & TCG_CALL_NO_SIDE_EFFECTS) {
                     for(i = 0; i < nb_oargs; i++) {
                         arg = args[i];
                         if (!dead_temps[arg] || mem_temps[arg]) {
@@ -1281,11 +1281,15 @@  static void tcg_liveness_analysis(TCGContext *s)
                         dead_temps[arg] = 1;
                         mem_temps[arg] = 0;
                     }
-                    
-                    if (!(call_flags & TCG_CALL_CONST)) {
+
+                    if (!(call_flags & TCG_CALL_NO_READ_GLOBALS)) {
+                        /* globals should be synced to memory */
+                        memset(mem_temps, 1, s->nb_globals);
+                    }
+                    if (!(call_flags & (TCG_CALL_NO_WRITE_GLOBALS |
+                                        TCG_CALL_NO_READ_GLOBALS))) {
                         /* globals should go back to memory */
                         memset(dead_temps, 1, s->nb_globals);
-                        memset(mem_temps, 1, s->nb_globals);
                     }
 
                     /* input args are live */
@@ -2081,10 +2085,14 @@  static int tcg_reg_alloc_call(TCGContext *s, const TCGOpDef *def,
             tcg_reg_free(s, reg);
         }
     }
-    
-    /* store globals and free associated registers (we assume the call
-       can modify any global. */
-    if (!(flags & TCG_CALL_CONST)) {
+
+    /* Save globals if they might be written by the helper, sync them if
+       they might be read. */
+    if (flags & TCG_CALL_NO_READ_GLOBALS) {
+        /* Nothing to do */
+    } else if (flags & TCG_CALL_NO_WRITE_GLOBALS) {
+        sync_globals(s, allocated_regs);
+    } else {
         save_globals(s, allocated_regs);
     }
 
diff --git a/tcg/tcg.h b/tcg/tcg.h
index fb6d9ea..a985b26 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -253,14 +253,24 @@  typedef int TCGv_i64;
 #define TCGV_UNUSED_I64(x) x = MAKE_TCGV_I64(-1)
 
 /* call flags */
-/* A pure function only reads its arguments and TCG global variables
-   and cannot raise exceptions. Hence a call to a pure function can be
-   safely suppressed if the return value is not used. */
-#define TCG_CALL_PURE           0x0010 
-/* A const function only reads its arguments and does not use TCG
-   global variables. Hence a call to such a function does not
-   save TCG global variables back to their canonical location. */
-#define TCG_CALL_CONST          0x0020
+/* Helper does not read globals (either directly or through an exception). It
+   implies TCG_CALL_NO_WRITE_GLOBALS. */
+#define TCG_CALL_NO_READ_GLOBALS    0x0010
+/* Helper does not write globals */
+#define TCG_CALL_NO_WRITE_GLOBALS   0x0020
+/* Helper can be safely suppressed if the return value is not used. */
+#define TCG_CALL_NO_SIDE_EFFECTS    0x0040
+
+/* convenience version of most used call flags */
+#define TCG_CALL_NO_RG          TCG_CALL_NO_READ_GLOBALS
+#define TCG_CALL_NO_WG          TCG_CALL_NO_WRITE_GLOBALS
+#define TCG_CALL_NO_SE          TCG_CALL_NO_SIDE_EFFECTS
+#define TCG_CALL_NO_RGSE        (TCG_CALL_NO_RG | TCG_CALL_NO_SE)
+#define TCG_CALL_NO_WGSE        (TCG_CALL_NO_WG | TCG_CALL_NO_SE)
+
+/* compatibility call flags, they should be eventually be removed */
+#define TCG_CALL_PURE           TCG_CALL_NO_SIDE_EFFECTS
+#define TCG_CALL_CONST          TCG_CALL_NO_READ_GLOBALS
 
 /* used to align parameters */
 #define TCG_CALL_DUMMY_TCGV     MAKE_TCGV_I32(-1)