Patchwork [3/3] tcg: declare internal helpers as const and pure

login
register
mail settings
Submitter Aurelien Jarno
Date March 4, 2010, 9:45 p.m.
Message ID <1267739110-26400-4-git-send-email-aurelien@aurel32.net>
Download mbox | patch
Permalink /patch/46985/
State New
Headers show

Comments

Aurelien Jarno - March 4, 2010, 9:45 p.m.
TCG internal helpers only access to the values passed in arguments, and
do not modify the CPU internal state. Thus they can be declared as
const and pure.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 tcg/tcg-op.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
Paul Brook - March 5, 2010, 11:15 a.m.
> TCG internal helpers only access to the values passed in arguments, and
> do not modify the CPU internal state. Thus they can be declared as
> const and pure.

I think this needs an explanatory comment. It's not immediately obvious that 
tcg_gen_helperN and tcg_gen_helper{32,64} have significantly different 
semantics.

tcg/README also needs updating, specifically:

"* Helpers:

Using the tcg_gen_helper_x_y it is possible to call any function
taking i32, i64 or pointer types. Before calling an helper, all
globals are stored at their canonical location and it is assumed that
the function can modify them. In the future, function modifiers will
be allowed to tell that the helper does not read or write some globals.
"

Paul
Aurelien Jarno - March 5, 2010, 8:07 p.m.
On Fri, Mar 05, 2010 at 11:15:45AM +0000, Paul Brook wrote:
> > TCG internal helpers only access to the values passed in arguments, and
> > do not modify the CPU internal state. Thus they can be declared as
> > const and pure.
> 
> I think this needs an explanatory comment. It's not immediately obvious that 
> tcg_gen_helperN and tcg_gen_helper{32,64} have significantly different 
> semantics.

What do you mean exactly? Mentioning explicitly tcg_gen_helper{32,64}
instead of "TCG internal helpers".

> tcg/README also needs updating, specifically:
> 
> "* Helpers:
> 
> Using the tcg_gen_helper_x_y it is possible to call any function
> taking i32, i64 or pointer types. Before calling an helper, all
> globals are stored at their canonical location and it is assumed that
> the function can modify them. In the future, function modifiers will
> be allowed to tell that the helper does not read or write some globals.
> "
> 

Fully agreed, but it is not a change introduce by this patch series. 
I'll submit a patch later.
Paul Brook - March 7, 2010, 10:39 p.m.
> On Fri, Mar 05, 2010 at 11:15:45AM +0000, Paul Brook wrote:
> > > TCG internal helpers only access to the values passed in arguments, and
> > > do not modify the CPU internal state. Thus they can be declared as
> > > const and pure.
> >
> > I think this needs an explanatory comment. It's not immediately obvious
> > that tcg_gen_helperN and tcg_gen_helper{32,64} have significantly
> > different semantics.
> 
> What do you mean exactly? Mentioning explicitly tcg_gen_helper{32,64}
> instead of "TCG internal helpers".

I think the difference between tcg_gen_helperN and tcg_gen_helper{32,64} is 
sufficiently subtle that it deserves documenting.  It's not obvious that the 
latter may only be used for cont/pure helpers.  My guess is that the FIXME 
you're removing was added precisely because there was uncertainty whether this 
assumption was reasonable, and under which circumstances they are used.

Paul

Patch

diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h
index c71e1a8..30a7a73 100644
--- a/tcg/tcg-op.h
+++ b/tcg/tcg-op.h
@@ -364,7 +364,6 @@  static inline void tcg_gen_helperN(void *func, int flags, int sizemask,
     tcg_temp_free_ptr(fn);
 }
 
-/* FIXME: Should this be pure?  */
 static inline void tcg_gen_helper32(void *func, TCGv_i32 ret,
                                     TCGv_i32 a, TCGv_i32 b)
 {
@@ -373,11 +372,11 @@  static inline void tcg_gen_helper32(void *func, TCGv_i32 ret,
     fn = tcg_const_ptr((tcg_target_long)func);
     args[0] = GET_TCGV_I32(a);
     args[1] = GET_TCGV_I32(b);
-    tcg_gen_callN(&tcg_ctx, fn, 0, 0, GET_TCGV_I32(ret), 2, args);
+    tcg_gen_callN(&tcg_ctx, fn, TCG_CALL_CONST | TCG_CALL_PURE,
+                  0, GET_TCGV_I32(ret), 2, args);
     tcg_temp_free_ptr(fn);
 }
 
-/* FIXME: Should this be pure?  */
 static inline void tcg_gen_helper64(void *func, TCGv_i64 ret,
                                     TCGv_i64 a, TCGv_i64 b)
 {
@@ -386,7 +385,8 @@  static inline void tcg_gen_helper64(void *func, TCGv_i64 ret,
     fn = tcg_const_ptr((tcg_target_long)func);
     args[0] = GET_TCGV_I64(a);
     args[1] = GET_TCGV_I64(b);
-    tcg_gen_callN(&tcg_ctx, fn, 0, 7, GET_TCGV_I64(ret), 2, args);
+    tcg_gen_callN(&tcg_ctx, fn, TCG_CALL_CONST | TCG_CALL_PURE,
+                  7, GET_TCGV_I64(ret), 2, args);
     tcg_temp_free_ptr(fn);
 }