IonMonkey: MIPS: Split shareable code to mips-shared in MacroAssembler-mips32

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: hev, Assigned: hev)

Tracking

Trunk
mozilla44
Other
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox44 fixed)

Details

Attachments

(2 attachments)

Bug 1194139, part 10: MacroAssembler-mips.
Depends on: 1205917
I understand merge MacroAssemblerMIPS to MacroAssemblerMIPSCompat is a bad way. I think use multiple class inheritance, too.

Multuple inheritance:
Assembler <- MacroAssemblerMIPSShared <- MacroAssemblerMIPSCompatShared
                                      \<- MacroAssemblerMIPS
MacroAssemblerMIPS              \
                                  <- MacroAssemblerMIPSCompat
MacroAssemblerMIPSCompatShared  /

So, Can I move MacroAssemblerMIPS to BaseMacroAssemblerMIPS, and MacroAssemblerMIPSCompat to MacroAssemblerMIPS?

Object Hierarchy:
Assember <- BaseMacroAssemblerMIPSShared <- BaseMacroAssemblerMIPS <- MacroAssemblerMIPSShared <- MacroAssemblerMIPS

Locations:
* Assembler in Assembler-mips32.
* BaseMacroAssemblerMIPSShared (aka shared parts of MacroAssemblerMIPS) in BaseMacroAssembler-mips-shared.
* BaseMacroAssemblerMIPS (aka arch-specific parts of MacroAssemblerMIPS) in BaseMacroAssembler-mips32.
* MacroAssemblerMIPSShared (aka shared parts of MacroAssemblerMIPSCompat) in MacroAssembler-mips-shared.
* MacroAssemblerMIPS (aka arch-specific parts of MacroAssemblerMIPSCompat) in MacroAssembler-mips32.

That means we needs use conditional macro to include Assembler-mips??.h in BaseMacroAssembler-mips-shared.h, and include BaseMacroAssembler-mips??.h in MacroAssember-mips-shared.h. What do you think? thanks!
Flags: needinfo?(nicolas.b.pierron)
I think you can follow what has been done on x86 / x64, i-e have the following class hierachy:

Assember <- AssemblerMIPSShared <- AssemblerMIPS32 <- MacroAssemblerMIPSShared <- MacroAssemblerMIPS32
Assember <- AssemblerMIPSShared <- AssemblerMIPS64 <- MacroAssemblerMIPSShared <- MacroAssemblerMIPS64

The way this is done, is by using typedef, and to inherit from the typedef.

Why do you need a new set of MacroAssembler class?  Would this be solved with asMasm() function?
Flags: needinfo?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> I think you can follow what has been done on x86 / x64, i-e have the
> following class hierachy:
> 
> Assember <- AssemblerMIPSShared <- AssemblerMIPS32 <-
> MacroAssemblerMIPSShared <- MacroAssemblerMIPS32
> Assember <- AssemblerMIPSShared <- AssemblerMIPS64 <-
> MacroAssemblerMIPSShared <- MacroAssemblerMIPS64
> 
> The way this is done, is by using typedef, and to inherit from the typedef.
> 
> Why do you need a new set of MacroAssembler class?  Would this be solved
> with asMasm() function?

Looks x86/x64 are simple and easy, because just one MacroAssembler. Did you remember i have tried merge MacroAssemblerMIPS to MacroAssemblerMIPSCompat in Bug 1199568? I don't need MacroAssemblerMIPS. Please see follow attachment, which one you prefer? Thanks!
Posted image class-hierachy.png
Sorry for the late answer,

I prefer the single flat hierarchy (third one from the left). Except that I will suggest to move the MacroAssemblerMIPSSharedCompat above in the class hierarchy.  This way you don't need extra files, and if you need extra function, then you duplicate the function in each macro assembler file, or you move it to the generic macro assembler.

Certainly not the second one from the left, as this would break the asMasm() function.
Depends on: 1211146
Thanks!
Attachment #8672961 - Flags: review?(nicolas.b.pierron)
Attachment #8672961 - Flags: review?(lhansen)
Attachment #8672961 - Flags: review?(lhansen) → review+
https://hg.mozilla.org/mozilla-central/rev/11969330ba7e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment on attachment 8672961 [details] [diff] [review]
Part 10: MacroAssembler-mips.

Review of attachment 8672961 [details] [diff] [review]:
-----------------------------------------------------------------

(Lars reviewed the patch)
Attachment #8672961 - Flags: review?(nicolas.b.pierron)
You need to log in before you can comment on or make changes to this bug.