I quite like this patch, it removes more code than it adds back. The results should look clear<br>and if we end wishing optimizing a spot we can trade the loop for an inlined function or macro. [1]<br><br>But there are a few things that I dislike about this patch.<br>
<br><br>--- a/mono/mini/cfold.c<br>+++ b/mono/mini/cfold.c<br>@@ -58,13 +58,18 @@ mono_is_power_of_two (guint32 val)<br>         res = (cast)arg1-&gt;inst_c0 op (cast)arg2-&gt;inst_c0;    \<br>         break; \<br> <br>+#define MONO_INST_NULLIFY_SREGS(dest) do {                \<br>
+        (dest)-&gt;sreg1 = (dest)-&gt;sreg2 = (dest)-&gt;sreg3 = -1;    \<br>+    } while (0)<br>+<br> #undef MONO_INST_NEW<br> #define MONO_INST_NEW(cfg,dest,op) do {    \<br>         (dest) = mono_mempool_alloc ((cfg)-&gt;mempool, sizeof (MonoInst));    \<br>
         (dest)-&gt;inst_p0 = (dest)-&gt;inst_p1 = (dest)-&gt;next = NULL; \<br>         (dest)-&gt;opcode = (op);    \<br>         (dest)-&gt;flags = 0; \<br>-        (dest)-&gt;dreg = (dest)-&gt;sreg1 = (dest)-&gt;sreg2 = -1;  \<br>
+        (dest)-&gt;dreg = -1;                    \<br>+    MONO_INST_NULLIFY_SREGS ((dest));            \<br>     } while (0)<br> <br>This is pure code duplication, kill it instead of patching.<br><br>@@ -138,6 +142,15 @@ ins_info[] = {<br>
 #include &quot;mini-ops.h&quot;<br> };<br> #undef MINI_OP<br>+#undef MINI_OP3<br>+<br>+#define MINI_OP(a,b,dest,src1,src2) -1,<br>+#define MINI_OP3(a,b,dest,src1,src2,src3) -1,<br>+gint8 ins_sreg_counts[] = {<br>+#include &quot;mini-ops.h&quot;<br>
+};<br>+#undef MINI_OP<br>+#undef MINI_OP3<br><br><br>This information can be calculated at compile time. All we get from running mini_init_op_sreg_counts<br>is an increased private RSS with no advantage. This information should be packaged in the ins_info array.<br>
<br><br>@@ -437,7 +450,7 @@ struct MonoInst {<br>     guint8  flags  : 5;<br>     <br>     /* used by the register allocator */<br>-    gint32 dreg, sreg1, sreg2;<br>+    gint32 dreg, sreg1, sreg2, sreg3;<br><br><br>This change increases the JIT working set significantly. Have you thought about doing something like<br>
struct MonoCallInst?<br><br>It would allow us to have 3 sregs and only pay the size price when needed. It would be trivial to have<br>a function to detect such instructions by expanding mini-ops.h to avoid hitting the ins_info array.<br>
<br>Besides that, I&#39;m eager to have this patch in so I can exploit it on Mono.Simd :)<br><br><br>[1] Doing something like like:<br>for (i = 0; i &lt; sreg_count (ins); ++i) <br>  zzzzz<br><br>to<br><br>static inline foo() { zzzzz } //OR<br>
<br>#define FOO() do { zzzzz; } while (0)<br><br>if (has_sreg1)<br>  foo ();<br>if (has_sreg2)<br>
  foo ();<br>if (unlikely (has_sreg3)<br> foo ();<br><br><br><br><div class="gmail_quote">2009/3/13 Mark Probst <span dir="ltr">&lt;<a href="mailto:mark.probst@gmail.com">mark.probst@gmail.com</a>&gt;</span><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im">On Mon, Feb 23, 2009 at 10:10 PM, Zoltan Varga &lt;<a href="mailto:vargaz@gmail.com">vargaz@gmail.com</a>&gt; wrote:<br>
&gt; All this to add support for exactly one rarely used instruction, CAS.<br>
<br>
</div>As per Paolo&#39;s request I timed a --compile-all of<br>
System.Windows.Forms.dll (which I chose instead of mscorlib.dll<br>
because it&#39;s larger) with and without the patch.  It turns out with<br>
the patch we&#39;re about 10% slower.  With the attached revised patch,<br>
which uses macros for all the getter functions, the difference is<br>
barely 3% (2.3166s vs 2.2522s, on x86).<br>
<font color="#888888"><br>
Mark<br>
</font><br>_______________________________________________<br>
Mono-devel-list mailing list<br>
<a href="mailto:Mono-devel-list@lists.ximian.com">Mono-devel-list@lists.ximian.com</a><br>
<a href="http://lists.ximian.com/mailman/listinfo/mono-devel-list" target="_blank">http://lists.ximian.com/mailman/listinfo/mono-devel-list</a><br>
<br></blockquote></div><br>