Currently all architectures are declaring their own MacroAssembler. This has 2 main issues: - Hard to follow what is unique in one architecture MacroAssembler. - Hard to highlight the fact that a CodeGen function can be promotted to a shared file, because it only depends on a common interface. What I am suggesting is to have one MacroAssembler structure which serve as a common interface. Each architecture will define the implementations of this MacroAssembler in different *.cpp files. The attached sample is just a dummy experiment of what I think we should aim for. The idea behind this example are: - A shared MacroAssembler.h, with multiple .cpp for implementing it for each architecture. (or one common one for implementing it for all architecture) - GodeGenerator cannot see neither AssemblerArch nor MacroAssemblerArch methods. - CodeGeneratorArch cannot see AssemblerArch methods. - Same syntax as before ( masm. … ) As soon as we settle on a design, we should incrementally migrate functions to this new model, and add related bugs as a blocker of this one.

As the solo guy trying to hold the OS X/ppc port together right now, how much refactoring are we talking? I still just have a PowerPC Baseline port; Ion still fails a lot of tests. Plus, on the larger scale, this seems less to remove the complexity and just moves it around to different places.

(In reply to Cameron Kaiser [:spectre] from comment #1) > As the solo guy trying to hold the OS X/ppc port together right now, how > much refactoring are we talking? I still just have a PowerPC Baseline port; > Ion still fails a lot of tests. Mostly alpha renaming and moving function declarations to other files. > Plus, on the larger scale, this seems less to remove the complexity and just > moves it around to different places. Indeed, we would not remove complexity, we would move it. Still the point is that I hope we can move as much as possible of this complexity such as the burden can be shared by multiple architectures instead of being duplicated on all architectures. Ideally, we should share as much as possible of this with Tier-1 platforms, such as when we fix something, we will also fix it for other architectures.

I have a few questions about this: 1. How do you plan to handle cases where most platforms have the same code and just one needs to specialize? My guess would be to Add #ifndef ARCH around the function in MacroAssembler.cpp and make another implementation in the MacroAssemblerArch.cpp 2. Do you plan on keeping MacroAssembler-x86-shared.cpp? Do you think there should be more of these files that are built for 2 or more platforms?

(In reply to Branislav Rankov from comment #3) > I have a few questions about this: > > 1. How do you plan to handle cases where most platforms have the same code > and just one needs to specialize? My guess would be to Add #ifndef ARCH > around the function in MacroAssembler.cpp and make another implementation in > the MacroAssemblerArch.cpp If this is this only one function, yes. If this is a list of function, and that they are extracted because of the same reason, then we should probably create a dedicated shared file for these functions. MacroAssembler-nunbox32.cpp MacroAssembler-punbox64.cpp I am confident that ARM and x86 have some functions in common, because of the Value representation. > 2. Do you plan on keeping MacroAssembler-x86-shared.cpp? Do you think there > should be more of these files that are built for 2 or more platforms? I think we should isolate common denominators. In such case the x86-64 is a close variant of x86. I don't know if we need to keep the MacroAssemblerX86 and MacroAssemblerX64, unless there is abstraction which are unique to each platform.

This is a great idea; we've had complaints from people that it's not clear which (macro-)assembler methods are available where. The attachment looks good overall. Luke and/or Waldo should probably also take a look, I'm not too familiar with private/protected inheritance.

Nicolas, how would this work with endianness? I've had to make a few hacks to Ion that I need to submit as patches, but I'll defer that if this is going to cover it.

Overall, I like the idea, but I'd make some tweaks: First, I very much like the idea of having a single class that has the list of all arch-independent methods with impls in different arch-specific .cpps. But I'd like to go further and get rid of *any* inheritance, so that we can see everyone in one class decl and avoid all the friending funny business. This will really cut down on the amount of time I constantly spend hunting down what is where. In particular: - The MacroAssembler/Assembler split has always been awkward and the placement of functions haphazard. I'd rather just flatten them into one so that we avoid having an "Ion" assembler and "JSC" assembler. - I think trying to break the arch-specific stuff into separate classes leads to a lot of cyclic-dependency problems that leads to the current 6-layer cake inheritance. As brutish and primitive as it sounds, I'd rather just have: class MacroAssembler { public: // arch-independent stuff #if defined(ARM) // arm stuff (probably with arm_ naming prefix) #elif defined(X64) // x86 stuff (probably with x64_ naming prefix) ... etc #endif private: // same as above, except for the private details }; Yes, it'll be a big file, but it's an assembler interface; it's not terribly complicated code preserving delicate invariants.

(In reply to Luke Wagner [:luke] from comment #7) > Overall, I like the idea, but I'd make some tweaks: > > First, I very much like the idea of having a single class that has the list > of all arch-independent methods with impls in different arch-specific .cpps. > But I'd like to go further and get rid of *any* inheritance, so that we can > see everyone in one class decl and avoid all the friending funny business. > This will really cut down on the amount of time I constantly spend hunting > down what is where. > > In particular: > - The MacroAssembler/Assembler split has always been awkward and the > placement of functions haphazard. I'd rather just flatten them into one so > that we avoid having an "Ion" assembler and "JSC" assembler. Whoa, getting rid of the Assembler and merging it into the MacroAssembler sounds like this would be a huge mess. The Assembler goal is to encode instructions, and nothing more. Doing this part is higly different between all architecture and there is no hope at merging them, except for x86 and x64, or MIPS N32 and MIPS O32. The Assembler handle Operands, Registers encoding and so. The MacroAssembler goal is to decompose into multiple assembly instructions, and collect information about the generated code, such as patchable locations, the stack depth, expressing ValueOperand, ImmGCPtr & ImmWord. The AssemblerX86 is not a good example of an Assembler, as it collects relocatable information, and this interface should be part of the MacroAssembler. "JSC" Assembler plus the Operand interface of "Ion" Assembler should be our new "Ion" Assembler, nothing more. Ideally, no CodeGenerator should ever use Assembler functions directly (even if we have to make a trivial dispatch). So I would see architecture-dependent parts of the MacroAssembler as a failure. One option would be to remove the inheritance and hide it under a private Assembler member. I agree, that ARM cannot be efficiently expressed with the same interface as x86. But I think we should put a clear name on these parts instead of naming them ARM or x86. One of the important difference which also appear on MIPS for example, is the separated destination register. I would highly prefer if we can define a macro such as "ASM_PREFER_SEPARATED_DESTINATION_REG" in Architecture-arch.h. This does not imply that we cannot implement ARM-prefered macro assembler with x86 instructions or the opposite, but this gives a choice for the rest of the engine. This kind of information is currently hidden under having separated CodeGenerator and having separated Lowering. I am not saying that we can express every Lowering with macros (as some instructions are expecting precise registers), but we should be able to reduce the duplication, and in addition, name the differences.

Yes, that's the theory, but it seems to be a lot messier in practice. I don't see why a whole separate class is required; it seems like the public interface of the MacroAssembler could just as well be implemented by a bunch of private member functions. I guess the really annoying part is the 6-deep inheritance hierarchy so, if nothing else, it would be preferable for the MacroAssembler to *contain* an Assembler.

(In reply to Luke Wagner [:luke] from comment #9) > Yes, that's the theory, but it seems to be a lot messier in practice. I > don't see why a whole separate class is required; it seems like the public > interface of the MacroAssembler could just as well be implemented by a bunch > of private member functions. I guess the really annoying part is the 6-deep > inheritance hierarchy so, if nothing else, it would be preferable for the > MacroAssembler to *contain* an Assembler. I was wondering, if we could just have a bunch of static functions within the cpp files, and move the assembler to the cpp file. The problem is that the Assembler still carry a state, such as the assembler buffer, or the last constant pool offset. Having a bunch of static variables implies that we have to carry around the assembler buffer as argument of every function which is producing some assembly code. Declaring the Assembler function in the MacroAssembler solve the problem of knowing which file you need to look at, but this implies that we would have to carry in one file multiple definitions of similar classes which are only enabled for one architecture. So at the end, when you will look for the definition of an Operand of the assembler you would be extremely confused because you will see 5 definitions in the same file and you would not know if this is the one that you need to patch. I do not want to see the code of ALL assemblers of every Architecture in one ENORMOUS file. Especially since the Assemblers have good reasons to be different between architectures. 2279 arm/Assembler-arm.h 1263 mips/Assembler-mips.h 755 shared/Assembler-shared.h 1696 shared/Assembler-x86-shared.h 780 x64/Assembler-x64.h 566 x86/Assembler-x86.h 7339 total I agree that we can avoid the inheritance, but we should not remove the Assembler. I think the best approach would be to make the assembler explicit in the MacroAssembler. // Contains a common interface for all architectures. (safe to be used by the CodeGenerator without any question) // Any #ifdef ARCH is a mistake class MacroAssembler { […] public: AssemblerArch &as(); private: AssemblerArch as_; }; The main detail would be that every direct access to the Assembler would require an explicit and verbose piece of code. masm.as().addu(dest, src); Having such explicit naming, without any friend based visibility implies that just by reading the name of the CodeGenerator-<arch>.cpp file, you know that you have to look into Assembler-<arch>.{h,cpp} file for the declaration / definition. Which should solve the issue you had about knowing where are functions, without packing everything into one file.