Re: [PATCH] tcg: refactor pool data for simplicity and comprehension

[Richard Henderson <richard.henderson@linaro.org>]

On 2/14/25 18:11, Michael Clark wrote:
> the intent of this patch is more conventional nomenclature
> but the constant pool data code is also simplified a little.
> 
> - merge new_pool_{alloc,insert} -> new_pool_data.
> - rename TCGLabelPoolData -> TCGData.
> - rename pool_labels -> pool_data.
> - rename macro TCG_TARGET_NEED_POOL_DATA.
> - move TCGData struct definition into tcg.h.
> - comment translation block epilogue members.

You can see from this list that this should be multiple patches.

> TCGLabelPoolData is ambiguous and asks for potential confusion
> with the unrelated TCGLabel type. there is no label in the sense
> of TCGLabel.

Fair.

> the label member is merely a pointer to the instruction text to
> be updated with the relative address of the constant, the primary
> data is the constant data pool at the end of translation blocks.
> this relates more closely to .data sections in offline codegen
> if we were to imagine a translation block has .text and .data.

No, it doesn't.  It relates most closely to data emitted within .text, accessed via 
pc-relative instructions with limited offsets.

This isn't a thing you'd have ever seen on x86 or x86_64, but it is quite common for arm32 
(12-bit offsets), sh4 (8-bit offsets), m68k (16-bit offsets) and such.  Because the 
offsets are so small, they could even be placed *within* functions not just between them.

> thus TCGData is more succinct and more reflective of what the
> structure contains; data emitted in the constant data pool at
> the end of translation blocks. also, pool_labels is renamed to
> pool_data as the primary contents of the list is constant data.

I guess.  TCGData is perhaps too short, but we can certainly avoid the confusion of "labels".

> 
> finally, new_pool_alloc and new_pool_insert are merged into a
> single function named new_pool_data, which moves nlongs to the
> end of the parameter list with varargs to allocate, copy, and
> insert constant data items to simplify new_pool_label et al.
> a successive step would be to collapse callers into calling
> new_pool_data and remove a layer of indirection.

Why?  varargs generally produces horrible code.
The split between alloc and insert was intentional to avoid this.

> diff --git a/tcg/tci/tcg-target.h b/tcg/tci/tcg-target.h
> index a9ca493d20f6..448c2330ef0f 100644
> --- a/tcg/tci/tcg-target.h
> +++ b/tcg/tci/tcg-target.h
> @@ -72,6 +72,6 @@ typedef enum {
>   } TCGReg;
>   
>   #define HAVE_TCG_QEMU_TB_EXEC
> -#define TCG_TARGET_NEED_POOL_LABELS
> +#define TCG_TARGET_NEED_POOL_DATA

Oops, this should have been removed with a417ef83.


r~