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

[Bruno Piazera Larsen <bruno.larsen@eldorado.org.br>]

[-- Attachment #1: Type: text/plain, Size: 6565 bytes --]

> > You are correct! I've just tweaked the code that defines spr_register and
> > it should be working now. I'm still working in splitting the SPR functions
> > from translate_init, since I think it would make it easier to prepare the
> > !TCG case and for adding new architectures in the future, and I found a
> > few more problems:
>
> Actually looking at the stuff below, I suspect that separating our
> "spr" logic specifically might be a bad idea.  At least some of the
> SPRs control pretty fundamental things about how the processor
> operates, and I suspect separating it from the main translation logic
> may be more trouble than it's worth.

Well, all the errors that I got were related to to read/write functions, which
I was already separating into a spr_tcg file. The solutions I can see are to
include this file in translate.c, and either have the read/write functions not be
static, or include the spr_common.c in translate as well, but only for TCG
builds. Both solutions sound pretty bad imo, but the first sounds less bad,
because it's a bit less complexity in the build process.

Other than that, I don't really see how we can support a !TCG build without
separating SPR, exactly because they are very fundamental to the processor.
Am I missing something? I fully expect to be, at this point, things are
turning out even more complicated than I expected



Bruno Piazera Larsen

Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>

Departamento Computação Embarcada

Analista de Software Trainee

Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>

________________________________
De: David Gibson
Enviadas: Quarta-feira, 21 de Abril de 2021 02:13
Para: Bruno Piazera Larsen
Cc: Fabiano Rosas; Thomas Huth; qemu-devel@nongnu.org; lagarcia@br.ibm.com; Lucas Mateus Martins Araujo e Castro; Fernando Eckhardt Valle; qemu-ppc@nongnu.org; Andre Fernando da Silva; Matheus Kowalczuk Ferst; Luis Fernando Fujita Pires
Assunto: Re: [PATCH 1/4] target/ppc: Code motion required to build disabling tcg

On Tue, Apr 20, 2021 at 07:02:36PM +0000, Bruno Piazera Larsen wrote:
> > > What I was doing was to only register the spr once, and use the
> > > accel-specific functions to set the relevant attributes, so spr_common
> > > wouldn't need to where (and if) spr_read_* exists or not.
> > > Would this work?
> > >
> > > Just ignoring the read and write functions means we still need
> > > to compile them, or at least stub them, otherwise we'd get linker
> > > problems.
> >
> > Not if you use a macro which will simply elide the references in the
> > !TCG case.  Actually I think even an inline wrapper will do it, I'm
> > pretty sure the compiler is smart enough to optimize the references
> > out in that case.
>
> You are correct! I've just tweaked the code that defines spr_register and
> it should be working now. I'm still working in splitting the SPR functions
> from translate_init, since I think it would make it easier to prepare the
> !TCG case and for adding new architectures in the future, and I found a
> few more problems:

Actually looking at the stuff below, I suspect that separating our
"spr" logic specifically might be a bad idea.  At least some of the
SPRs control pretty fundamental things about how the processor
operates, and I suspect separating it from the main translation logic
may be more trouble than it's worth.

> 1. Global variables being defined in translate.c and used both there and
> in spr functions. The variables in question are: cpu_gpr, cpu_so, cpu_ov,
> cpu_ca, cpu_ov32, cpu_ca32, cpu_xer, cpu_lr and cpu_ctr. The easy way
> out is to have global "getters" and "setters" for those, but I'm not sure
> it's a good solution.

Yeah, that seems really messy, plus those are special variables used
by TCG generated code whose operation I don't really understand.

> 2. Static functions defined in translate.c, used there and TCG-related
> spr functions. They are: gen_load_spr, gen_store_spr, gen_stop_exception,
> gen_inval_exception. The easy solution is adding a prototype to internal.h
> and removing the static, but again, not sure it's a good solution

I think gen_*() functions should stay in translate.c, since they are,
explicitly, about translation ("gen" for "generating code").

> 3. gen_read_xer (currently in spr_common) calls is_isa300, defined in
> include/disas/disas.h, which is a macro that dereferences DisasContext.
> However, the struct is defined in translate.c. This one is pretty easy, I think,
> it's just turning the macro into a function, but since I'm already e-mailing,
> I figured I might as weel ask.
>
> Finally, since most read and write functions are static, I added them to
> spr_tcg.c.inc to be included only with TCG, as a quick fix, but I would really
> prefer some other solution if there is anything better. Any thoughts on this?
>
> IIRC, this is the last thing holding me back from an RFC with this motion
> patch
>
>
> Bruno Piazera Larsen
>
> Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>
>
> Departamento Computação Embarcada
>
> Analista de Software Trainee
>
> Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>

--
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: text/html, Size: 12042 bytes --]