[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC][PATCH v2 3/4] tcg: add optimized TCG qemu_ld/st g

From: Yeongkyoon Lee
Subject: Re: [Qemu-devel] [RFC][PATCH v2 3/4] tcg: add optimized TCG qemu_ld/st generation
Date: Fri, 06 Jul 2012 20:20:52 +0900
User-agent: Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20120615 Thunderbird/13.0.1

Is it really worth having this as a CONFIG_ switch? If we think
it's better to do this out of line we should just switch to
always generating the out of line code, I think. There's not much
point in retaining the old code path if it's disabled -- it will
just bitrot.

I agree.
However, it is just a safe guard because I have not test all the targets of qemu.
I've only tested x86 and ARM targets on x86 and x86-64 hosts.
If agreed to remove conditional macro, then I'll fix it.

+    /* jne slow_path */
+    tcg_out_opc(s, OPC_JCC_long + JCC_JNE, 0, 0, 0);
+    if (!label_ptr) {
+        tcg_abort();
+    }
There's no point in this check and abort -- label_ptr will always be non-NULL (it would be an internal error if it wasn't), and if it is by some future bug NULL, we'll just crash on the next line, which is just as good. The existing code didn't feel the need to make this check, we don't need to do it in the new code.

It cannot be happened now as you said. It is just for a possible future bug.
But I cannot understand "we'll just crash on the next line" you mentioned above.

+    /* helper stub will be jumped back here */
"will jump back here".


+    /* helper stub will be jumped back here */


+/* optimization to reduce jump overheads for qemu_ld/st IRs */
+ * qemu_ld/st code generator call add_qemu_ldst_label,
+ * so that slow case(TLB miss or I/O rw) is handled at the end of TB
+ */
This comment isn't really describing the purpose of this function,
which is something more along the lines of "Record the context of
a call to the out of line helper code for the slow path for a
load or store, so that we can later generate the correct helper

I agree. Your description looks better.

+    if (s->nb_qemu_ldst_labels >= TCG_MAX_QEMU_LDST)
+        tcg_abort();
QEMU coding style requires braces. Please use checkpatch.pl.


+    idx = s->nb_qemu_ldst_labels++;
+    label = (TCGLabelQemuLdst *)&s->qemu_ldst_labels[idx];
+    label->opc_ext = opc_ext;
+    label->datalo_reg = data_reg;
+    label->datahi_reg = data_reg2;
+    label->addrlo_reg = addrlo_reg;
+    label->addrhi_reg = addrhi_reg;
+    label->mem_index = mem_index;
+    label->raddr = raddr;
+    if (!label_ptr) {
+        tcg_abort();
+    }
Another pointless abort.


diff --git a/tcg/tcg.c b/tcg/tcg.c
index 8386b70..8009069 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -301,6 +301,14 @@ void tcg_func_start(TCGContext *s)

      gen_opc_ptr = gen_opc_buf;
      gen_opparam_ptr = gen_opparam_buf;
+    /* initialize qemu_ld/st labels which help to generate TLB miss case codes 
at the end of TB */
+    s->qemu_ldst_labels = tcg_malloc(sizeof(TCGLabelQemuLdst) * 
+    if (!s->qemu_ldst_labels) {
+        tcg_abort();
+    }
Unnecessary check -- tcg_malloc() can never return 0.


+/* Macros and structures for qemu_ld/st IR code optimization:
+   It looks good for TCG_MAX_HELPER_LABELS to be half of OPC_BUF_SIZE in 
exec-all.h. */
+#define TCG_MAX_QEMU_LDST       320
Is that true even if you have a huge block with nothing but simple
guest load instructions in it?

I agree. It needs to be set as same size with OPC_BUF_SIZE for covering extreme cases.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]