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.
+#ifdef CONFIG_QEMU_LDST_OPTIMIZATION
+ /* 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.
+#if defined(CONFIG_QEMU_LDST_OPTIMIZATION)
+ /* helper stub will be jumped back here */
"will jump back here".
Ok.
+#if defined(CONFIG_QEMU_LDST_OPTIMIZATION)
+ /* helper stub will be jumped back here */
ditto.
Ok.
+#if defined(CONFIG_QEMU_LDST_OPTIMIZATION)
+/* 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
code".
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.
Ok.
+
+ 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.
ditto.
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;
+#if defined(CONFIG_QEMU_LDST_OPTIMIZATION)
+ /* 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) *
TCG_MAX_QEMU_LDST);
+ if (!s->qemu_ldst_labels) {
+ tcg_abort();
+ }
Unnecessary check -- tcg_malloc() can never return 0.
Ok.
+#if defined(CONFIG_QEMU_LDST_OPTIMIZATION)
+/* 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.
