Re: [PATCH 15/16] system/memory: make compilation unit common

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 3/10/25 11:27, Philippe Mathieu-Daudé wrote:
> On 10/3/25 19:04, Pierrick Bouvier wrote:
>> On 3/10/25 10:58, Richard Henderson wrote:
>>> On 3/10/25 10:47, Pierrick Bouvier wrote:
>>>>> Maybe better as
>>>>>
>>>>>         MemOp mop = (target_words_bigendian() ? MO_BE : MO_LE) |
>>>>> size_memop(size);
>>>>>         adjust_endianness(mr, &mrfd.data, size_memop(size), mop);
>>>>>
>>>>
>>>> Do you think defining MO_TE as this expression is a good idea?
>>>
>>> There are not so many references to MO_TE outside target/ or accel/tcg/.
>>>
>>> Indeed, after this change, the only ones left are in hw/arm/armv7m.c,
>>> which (because it's Arm) can be changed to MO_LE.
>>>
>>
>> I see a bit more than that (17 files):
>> hw/arm/armv7m.c
>> include/exec/memop.h
>> target/arm/tcg/helper-a64.c
>> target/arm/tcg/translate.c
>> target/hexagon/idef-parser/parser-helpers.c
>> target/hppa/translate.c
>> target/i386/tcg/emit.c.inc
>> target/loongarch/tcg/insn_trans/trans_vec.c.inc
>> target/m68k/translate.c
>> target/mips/tcg/mips16e_translate.c.inc
>> target/riscv/translate.c
>> target/rx/translate.c
>> target/s390x/tcg/mem_helper.c
>> target/s390x/tcg/translate.c
>> target/s390x/tcg/translate_vx.c.inc
>> target/sparc/ldst_helper.c
>> target/sparc/translate.c
> 
> For targets tied to single endianness, we can replace using gsed,
> but using a helper is clearer (see for example commit 415aae543ed
> target/microblaze: Consider endianness while translating code").
> 

That's good, I just want to keep this series focus on minimal changes to 
achieve the current goal, and not bring any additional refactoring here.

>> Plus more (22 files) who relies on:
>> MO_TE* variants (which relies on MO_TE transitively)
>>
>> Thus my proposal to have a first change to MO_TE definition, and
>> eventually do the change later.
>>
>> What do you think?
> 
> Removing MO_TE is in my TODO list.
> 
> I started with Microblaze (now merged) to get familiar, then had
> a look at ARM (see i.e.
> https://lore.kernel.org/qemu-devel/20240924191932.49386-1-philmd@linaro.org/
> and
> https://lore.kernel.org/qemu-devel/d831600a-9a61-45c1-a535-f75bb64cdff4@linaro.org/).
> I also took care of MIPS few years ago but I need to rebase,
> however it isn't in the priority list.

Instead of doing a full codebase refactoring/cleanup, we can adopt a "as 
needed basis" approach.

Basically architectures that can have varying endianness must be handled 
(since their compilation units are duplicated for variants).
For the rest, as Richard mentioned on this series, the code will stay 
target specific, compiling with a single set of defines, which is what 
we really aim for.

Same discussion will happen for architectures with files duplicated 
between 32/64 bits variants.