[Mono-dev] Patch: Ternary ops in mini and general ATOMIC_CAS

[Rodrigo Kumpera]

I quite like this patch, it removes more code than it adds back. The results
should look clear
and if we end wishing optimizing a spot we can trade the loop for an inlined
function or macro. [1]

But there are a few things that I dislike about this patch.


--- a/mono/mini/cfold.c
+++ b/mono/mini/cfold.c
@@ -58,13 +58,18 @@ mono_is_power_of_two (guint32 val)
         res = (cast)arg1->inst_c0 op (cast)arg2->inst_c0;    \
         break; \

+#define MONO_INST_NULLIFY_SREGS(dest) do {                \
+        (dest)->sreg1 = (dest)->sreg2 = (dest)->sreg3 = -1;    \
+    } while (0)
+
 #undef MONO_INST_NEW
 #define MONO_INST_NEW(cfg,dest,op) do {    \
         (dest) = mono_mempool_alloc ((cfg)->mempool, sizeof (MonoInst));
\
         (dest)->inst_p0 = (dest)->inst_p1 = (dest)->next = NULL; \
         (dest)->opcode = (op);    \
         (dest)->flags = 0; \
-        (dest)->dreg = (dest)->sreg1 = (dest)->sreg2 = -1;  \
+        (dest)->dreg = -1;                    \
+    MONO_INST_NULLIFY_SREGS ((dest));            \
     } while (0)

This is pure code duplication, kill it instead of patching.

@@ -138,6 +142,15 @@ ins_info[] = {
 #include "mini-ops.h"
 };
 #undef MINI_OP
+#undef MINI_OP3
+
+#define MINI_OP(a,b,dest,src1,src2) -1,
+#define MINI_OP3(a,b,dest,src1,src2,src3) -1,
+gint8 ins_sreg_counts[] = {
+#include "mini-ops.h"
+};
+#undef MINI_OP
+#undef MINI_OP3


This information can be calculated at compile time. All we get from running
mini_init_op_sreg_counts
is an increased private RSS with no advantage. This information should be
packaged in the ins_info array.


@@ -437,7 +450,7 @@ struct MonoInst {
     guint8  flags  : 5;

     /* used by the register allocator */
-    gint32 dreg, sreg1, sreg2;
+    gint32 dreg, sreg1, sreg2, sreg3;


This change increases the JIT working set significantly. Have you thought
about doing something like
struct MonoCallInst?

It would allow us to have 3 sregs and only pay the size price when needed.
It would be trivial to have
a function to detect such instructions by expanding mini-ops.h to avoid
hitting the ins_info array.

Besides that, I'm eager to have this patch in so I can exploit it on
Mono.Simd :)


[1] Doing something like like:
for (i = 0; i < sreg_count (ins); ++i)
  zzzzz

to

static inline foo() { zzzzz } //OR

#define FOO() do { zzzzz; } while (0)

if (has_sreg1)
  foo ();
if (has_sreg2)
  foo ();
if (unlikely (has_sreg3)
 foo ();



2009/3/13 Mark Probst <mark.probst at gmail.com>

> On Mon, Feb 23, 2009 at 10:10 PM, Zoltan Varga <vargaz at gmail.com> wrote:
> > All this to add support for exactly one rarely used instruction, CAS.
>
> As per Paolo's request I timed a --compile-all of
> System.Windows.Forms.dll (which I chose instead of mscorlib.dll
> because it's larger) with and without the patch.  It turns out with
> the patch we're about 10% slower.  With the attached revised patch,
> which uses macros for all the getter functions, the difference is
> barely 3% (2.3166s vs 2.2522s, on x86).
>
> Mark
>
> _______________________________________________
> Mono-devel-list mailing list
> Mono-devel-list at lists.ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20090318/6135b975/attachment.html