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

[David Gibson <david@gibson.dropbear.id.au>]

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

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: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]