Implement mffscdrn[i] and move mffs* to decodetree #65
Conversation
vcoracolombo
force-pushed
the
vccolombo-mffscdrn
branch
from
c95a145 to
7ca8ecf
Compare
vcoracolombo
force-pushed
the
vccolombo-mffscdrn
branch
from
7ca8ecf to
d754f0c
Compare
There was a problem hiding this comment.
Moving old instructions first may give you more readable patches. Also, handling simpler instructions first can help. I'd suggest the following order for this patch series:
- target/ppc: Fix insn32.decode style issues
- target/ppc: Move mffs[.] to decodetree
- target/ppc: Move mffsl to decodetree
- target/ppc: Move mffsce to decodetree
- target/ppc: Move mffscrn[i] to decodetree
- target/ppc: Implement mffscdrn[i] instructions
So you can add do_mffsc parameters incrementally.
target/ppc/translate/fp-impl.c.inc
Outdated
| tcg_gen_andi_i64(fpscr, fpscr, clear_mask); | ||
| tcg_gen_or_i64(fpscr, fpscr, t1); | ||
|
|
||
| gen_helper_store_fpscr(cpu_env, fpscr, tcg_constant_i32(fpscr_mask)); |
There was a problem hiding this comment.
Don't call gen_helper_store_fpscr if the instruction is not setting FPSCR.
(Also, we should probably resolve the TODO in this helper...)
There was a problem hiding this comment.
Sorry, I don't understand the issue with gen_helper_store_fpscr here. Could you give an example?
There was a problem hiding this comment.
trans_MFFS and trans_MFFSL call do_mffsc, but they don't set FPSCR, so the call to gen_helper_store_fpscr is unnecessary. You can use set_mask/clear_mask values or add another parameter to control whether the helper should be called. Or split do_mffsc in two methods.
vcoracolombo
force-pushed
the
vccolombo-mffscdrn
branch
from
d754f0c to
2f5e0f9
Compare
target/ppc/translate/fp-impl.c.inc
Outdated
| tcg_temp_free_i64(t0); | ||
| } | ||
|
|
||
| static void do_mffsc(int rt, TCGv_i64 t1, uint64_t set_mask) |
There was a problem hiding this comment.
It's odd to have t1 unused and the only caller using set_mask as 0xFFFFFFFFFFFFFFFFULL. It's obvious from the following patches, but reviewers will read the patch series sequentially. I think it'd be better to add set_mask only when you move mffsl (or move it in the same commit) and t1 when you move mffsce.
There was a problem hiding this comment.
Implemented the changes you suggested, thanks again!
There was a problem hiding this comment.
It's looking better, but you should also drop the tcg_gen_andi_i64 with 0xFFFFFFFFFFFFFFFFULL in the second patch.
There was a problem hiding this comment.
Yeah, my bad, didn't notice it. Fixed
vcoracolombo
force-pushed
the
vccolombo-mffscdrn
branch
from
2f5e0f9 to
14db33d
Compare
Signed-off-by: Víctor Colombo <[email protected]>
Signed-off-by: Víctor Colombo <[email protected]>
Signed-off-by: Víctor Colombo <[email protected]>
Signed-off-by: Víctor Colombo <[email protected]>
Signed-off-by: Víctor Colombo <[email protected]>
Signed-off-by: Víctor Colombo <[email protected]>
vcoracolombo
force-pushed
the
vccolombo-mffscdrn
branch
from
14db33d to
9ed4c5c
Compare
2 participants
No description provided.