Closed
Bug 1205134
Opened 9 years ago
Closed 9 years ago
IonMonkey: MIPS: Split shareable code to mips-shared in MacroAssembler-mips32
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla44
People
(Reporter: hev, Assigned: hev)
References
Details
Attachments
(2 files)
134.07 KB,
image/png
|
Details | |
189.82 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
Bug 1194139, part 10: MacroAssembler-mips.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
(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!
Assignee | ||
Comment 4•9 years ago
|
||
Comment 5•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Attachment #8672961 -
Flags: review?(lhansen)
Updated•9 years ago
|
Attachment #8672961 -
Flags: review?(lhansen) → review+
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/11969330ba7e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment 9•9 years ago
|
||
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.
Description
•