1121613 - Clean-up: Move MacroAssemblerSpecific::Push to the generic MacroAssembler.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Let's merge the declarations of Push & Pop functions …

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Move MacroAssemblerSpecific::Push to the generic MacroAssembler.

This is a large patch but the modifications are mostly moving code.
The patch does the following things:

- Add private asMasm() functions to specific macro assemblers.  This is
   using private, and not protected, such that the generic MacroAssembler
   cannot use this functions.

- Create a section for the functions which are manipulating the stack.  Move
  all related functions into this category, even if they were already
  defined in the file.

- Add a macro PER_ARCH to hint that the MacroAssembler function is defined
  in each Macro Assembler files, or in MacroAssembler.cpp

- Add a macro ONLY_X86_X64 which is defined as "= delete" for other
  architectures, which provides documentation and error messages :)

- Move the body of functions to the MacroAssembler*.cpp (Bug 837070), and
  also for functions which are using the asMasm() function, such as
  CallWithExitFrame.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8549083 [details] [diff] [review]
Move MacroAssemblerSpecific::Push to the generic MacroAssembler.

Requesting feedback from people who likely have thoughts on this as well.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8549083 [details] [diff] [review]
Move MacroAssemblerSpecific::Push to the generic MacroAssembler.

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

I haven't thought through all the consequences, but I really like seeing all the signatures together in MacroAssembler.h, so it looks great to me so far.  Nice job nbp!

Comment 4

[Dan Gohman [:sunfish]]

Comment on attachment 8549083 [details] [diff] [review]
Move MacroAssemblerSpecific::Push to the generic MacroAssembler.

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

Overall looks good. Please add a comment on asMasm() mentioning that it's performing a downcast.

At the same time, I also want to explore the space a little bit. First, for my benefit, here's the class hierarchy on x64:

class AssemblerShared
class AssemblerX86Shared      : public AssemblerShared
class Assembler               : public AssemblerX86Shared
class MacroAssemblerX86Shared : public Assembler
class MacroAssemblerX64       : public MacroAssemblerX86Shared
typedef MacroAssemblerX64 MacroAssemblerSpecific;
class MacroAssembler          : public MacroAssemblerSpecific

Ok. So let's consider one of the functions in the patch, say this one:

    void Push(const ValueOperand &val);

It's currently in MacroAssemblerX64. The patch here moves it down to MacroAssembler. But we still need to call it from MacroAssemblerX86Shared and MacroAssemblerX64, so then the patch has asMasm() for downcasting.

Can we move this Push up to Assembler instead? No, because it's Macro-ish.

Can we introduce a new class MacroAssemblerShared, which would go between Assembler and MacroAssemblerX86Shared in the inheritance graph, and then move this Push there? No, because this Push calls:

    void pushValue(ValueOperand val)

which is declared in MacroAssemblerX64.

Can we make pushValue portable and move it into our hypothetical MacroAssemblerShared? It doesn't vary a lot between platforms (there's mainly just NUNBOX32 and PUNBOX64), and we theoretically could do it, but there's also push vs sub+store and maybe some other details, and it's just tricky enough that it's probably not worth it.

Ok, what if we move the *declaration* of pushValue() into MacroAssemblerShared, and then have platform-specific definitions? So we'd have

    // In class MacroAssemblerShared, in shared/MacroAssemblerShared.h
    void pushValue(ValueOperand val);

    // In x64/MacroAssemblerShared-x64.cpp
    void MacroAssemblerShared::pushValue(ValueOperand val) {
       ...
    }

    // In arm/MacroAssemblerShared-arm.cpp
    void MacroAssemblerShared::pushValue(ValueOperand val) {
       ...
    }

That would seem to work. I don't know if it would work for everything, but it would work for the handful of things I looked into. We could move all the Push code into MacroAssemblerShared and get rid of the downcasting. Thoughts?

At the same time, I realize that this is a pretty big change, and I don't want to hold up the patch here, because it is an improvement over the status quo. So feedback+.

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Dan Gohman [:sunfish] from comment #4)
> Overall looks good. Please add a comment on asMasm() mentioning that it's
> performing a downcast.

The ultimate goal is to be able to remove this function, after having remove the hierachy of MacroAssembler.

> At the same time, I also want to explore the space a little bit. First, for
> my benefit, here's the class hierarchy on x64:
> 
> class AssemblerShared
> class AssemblerX86Shared      : public AssemblerShared
> class Assembler               : public AssemblerX86Shared
> class MacroAssemblerX86Shared : public Assembler
> class MacroAssemblerX64       : public MacroAssemblerX86Shared
> typedef MacroAssemblerX64 MacroAssemblerSpecific;
> class MacroAssembler          : public MacroAssemblerSpecific

Which is why I want to have one MacroAssembler for all platform, and mark the functions as deleted if they do not exists on this platform, but such deleted functions should be the exception.

Then we have multiple implementation, and ideally, the header file should index where the implementation goes (PER_ARCH macro), I don't know to which extend I can write a compiler test which ensure that such annotation is honored.

> Ok, what if we move the *declaration* of pushValue() into
> MacroAssemblerShared, and then have platform-specific definitions? So we'd
> have
> 
>     // In class MacroAssemblerShared, in shared/MacroAssemblerShared.h
>     void pushValue(ValueOperand val);
> 
>     // In x64/MacroAssemblerShared-x64.cpp
>     void MacroAssemblerShared::pushValue(ValueOperand val) {
>        ...
>     }
> 
>     // In arm/MacroAssemblerShared-arm.cpp
>     void MacroAssemblerShared::pushValue(ValueOperand val) {
>        ...
>     }
> 
> That would seem to work. I don't know if it would work for everything, but
> it would work for the handful of things I looked into. We could move all the
> Push code into MacroAssemblerShared and get rid of the downcasting. Thoughts?

What you call MacroAssemblerShared, is what I call MacroAssembler.  I do not see any need for a macro assembler if it cannot be shared between platforms, and if some functions are specific to one architecture, then they should be marked as deleted for the others.

The goal of the MacroAssembler is to make the Assembler invisible as much as possible, and thus the platform specific leakage should be limited to an interface provided by the MacroAssembler.  As luke suggested in Bug 996602, we should remove the inheritance from the Assembler.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8549083 [details] [diff] [review]
Move MacroAssemblerSpecific::Push to the generic MacroAssembler.

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

Sorry for the delay, looks great.

::: js/src/jit/MacroAssembler.h
@@ +29,5 @@
>  #include "vm/ProxyObject.h"
>  #include "vm/Shape.h"
>  
> +// This Macro is only a hint for code readers that the function is defined in a
> +// specific macro assebler, and not in the generic macro assembler.  Thus, when

Nit: assembler (typo)

::: js/src/jit/shared/MacroAssembler-x86-shared.cpp
@@ +272,5 @@
> +void
> +MacroAssembler::Push(Register reg)
> +{
> +    push(reg);
> +    framePushed_ += sizeof(intptr_t);

Should we move these simple methods into MacroAssembler.h (or MacroAssembler-inl.h or something)?

Maybe it doesn't really matter (I know the ARM assembler does this too), but for these small functions it seems inlining could be beneficial...

Comment 7

[Nicolas B. Pierron [:nbp]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd61b59dfc95
inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/97a152aae8c0

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/97a152aae8c0