[Top][All Lists]

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

RE: [PATCH 1/4] target/ppc: Code motion required to build disabling tcg

From: Fabiano Rosas
Subject: RE: [PATCH 1/4] target/ppc: Code motion required to build disabling tcg
Date: Mon, 12 Apr 2021 10:56:23 -0300

Bruno Piazera Larsen <bruno.larsen@eldorado.org.br> writes:

>> > This commit does the necessary code motion from translate_init.c.inc
>> For instance, I don't immediately see why these changes are necessary. I
>> see that translate_init.c.inc already has some `#ifdef CONFIG_TCG`, so
>> why do we need to move a bunch of code into cpu.c instead of just adding
>> more code under ifdef CONFIG_TCG? (I'm not saying it's wrong, just trying to
>> understand the reasoning).
> There are 3 main reasons for this decision. The first is kind of
> silly, but when I read translate.c my mind jumped to translating
> machine code to TCG, and the amount of TCGv variables at the start
> reinforced this notion.  The second was that both s390x and i386
> removed it (translate.c) from compilation, so I had no good reason to
> doubt it.  The last (and arguably most important) is that translate.c
> is many thousands of lines long (translate_init.c.inc alone was almost
> 11k). The whole point of disabling TCG is to speed up compilation and
> reduce the final file size, so I think it makes sense to remove that
> big file.  And the final nail in the coffin is that at no point did it
> cross my mind to keep the init part of translation, but remove the
> rest

Ok, so whatever you decide to do, make sure you state: "this is where
TCG functions go", "this file is built on KVM|TCG only", and so on. And
it's ok in principle to do a partial move for the RFC, but please
mention that somewhere so we're all in the same page.

> Also, I'm not a fan of big ifdefs, because it's kinda hard to follow
> them when viewing code in vim. Adding that to already having a cpu.c
> file, where it makes sense (to me, at least) to add all CPU related
> functions, just sounded like a good idea.

My point about ifdefs is that it would be easier to understand if you

a) wrapped code in ifdefs, got the RCF going and then later moved all
ifdef'ed code into a new tcg-only file;


b) define which is the tcg-only file right away and just move code in

When you moved code into cpu.c *along with* the ifdefs, you kind of did
both at the same time, which got confusing; to me at least.

About moving CPU related functions into cpu.c, that's fine. But it is
not strictly related to disable-tcg, so maybe send that in a separate
patch that does it explicitly at the start of the series.

>> Is translate_init.c.inc intended to be TCG only? But then I see you
>> moved TCG-only functions out of it (ppc_fixup_cpu) and left not TCG-only
>> functions (gen_spr_generic).
> This is me misjudging what is TCG and what is not, mostly. I think
> that actually moving everything to cpu.c and adding ifdefs, or
> creating a cpu_tcg.c.inc or similar, would be the most sensible code
> motion, but every function I removed from translate gave me 3 new
> errors, at some point I felt like I should leave something behind
> otherwise we're compiling everything from TCG again, just in different
> files, so I tried to guess what was TCG and what was not (also, I
> really wanted the RFC out by the weekend, I _may_ have rushed a few
> choices).

Ok, so you left out some things on purpose to reduce complexity for the
first RFC. That's fine, we just need to state these kinds of thing more

>> > This moves all functions that start with gdb_* into target/ppc/gdbstub.c
>> > and creates a new function that calls those and is called by 
>> > ppc_cpu_realize
>> This looks like it makes sense regardless of disable-tcg, could we have
>> it in a standalone patch?
> Sure, I'll try and get it ready ASAP, and make sure I didn't miss one
> function before sending. Just a noob question... do I edit the patch
> manually to have it only contain the gdb code motion, or is there a
> way to move back to before I actually made the commit without needing
> to re-do the changes?

You could git rebase -i back into your first patch and then split it in
two (git reset HEAD~1 and stage + commit each one), one for moving CPU
code into cpu.c and other for moving GDB code into gdbstub.c.

Or just checkout a new branch, apply the patch on top of it and commit
just the GDB change.

Feel free to ping me on IRC if you need help.

reply via email to

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