Closed Bug 1184959 Opened 4 years ago Closed 4 years ago

Move MacroAssembler ABI calls functions to the generic macro assembler.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox42 --- affected
firefox43 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 1 obsolete file)

Move passABIArg, callWithABI, and setup*ABICall to the generic MacroAssembler.
Here is some explanations of the current approach:

Some far we have multiple way to handle ABI function calls.  The first one is the one from the MacroAssembler, with setupUnaligned…, passABIArg, and callWithABI.  The second, is the one added with AsmJS, which has a ABIArgGenerator, and an ABIArg structure.

IMHO, the second interface is nicer than the first one, as it is not coupled with the MoveResolver, and because it does only one thing.

The intent is to re-use ABIArgGenerator to simplify passABIArg, and isolate as much as possible of the architecture specific part.
This patch removes the default values, such as '= MoveOp::GENERAL' from the
signature of the functions.
Attachment #8637206 - Flags: review?(hv1989)
Do not display an 'inline' keyword on the signatures, if the function are
not inlined.
Attachment #8637208 - Flags: review?(hv1989)
This patch moves the logic from MacroAssemblerARMCompat::passWithABI to
ABIArgGenerator, and untie it from the MoveResolver / Simulator logic.

The official documentation is available on arm.com website[1].

[1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ihi0042e/index.html
Attachment #8637211 - Flags: review?(hv1989)
The is to let the ABIArgGenerator generates the MoveOperand, within
passWithABI.

This patch add support for register pairs in the MoveResolver, and in the
MoveEmitter-arm.  This basically moves the logic for moving registers from
the CallWithABIPre into the MoveEmitter.

In addition, I added a ma_vxfer, to handle float transfers in
CallWithABIPost, and in the MoveEmitter.  This avoid having to rely on the
as_* functions when similar variants already exists.
Attachment #8637212 - Flags: review?(hv1989)
Update the ABIArgGenerator to use register pairs to encode double arguments,
if needed.  Unfortunately, we should stay out of register pairs for the
moment, as we do not yet have the infrastructure to express it properly in
the Lowering phase.

This modification is made to prepare the remove of passABIArg, which will
now use the ABIArgGererator on all architectures.
Attachment #8637880 - Flags: review?(branislav.rankov)
This patch is large because it does 5 times the same things.  For this
reason I am asking 5 persons, such that each can carefully review one
architecture:

 - x86: bbouvier (compiles, and pass all tests)
 - x64: jandem (compiles, and pass all tests)
 - ARM: h4writer (compiles, and pass all tests)
 - MIPS: rankov (compiles, fails ES7 new.target tests)
 - ARM64: sstangl (does not compile due to existing errors, I will try a rebase)

This patch does the following things:
 - Move some MacroAssemblerSpecific variables into the MacroAssembler, such
   as inCall_, dynamicAlignment_, and signature_ (renamed field of
   passedArgTypes_)

 - It removes args_, and passedArgs_ fields, which were used only for
   assertions, and never got reported any recently.  Thus remove the
   setupABICall argument.  Others 'args' arguments of setupAlignedABICall
   and setupUnalignedABICall will be removed in a follow-up patch.

 - passABIArg is now architecture indepedent and it uses the
   ABIArgGenerator, which basically does the same thing as what passABIArg
   used to do.  This help remove all the local variables which are
   architecture specific, by aggregating a structure which is architecture
   specific. Note, the previous patches were made to ensure that
   ABIArgGenerator generates ABI compliant alllocations.

 - callWithABIPre, when no dynamic alignment is made, accounts / asserts for
   the prologue of AsmJS frames, which are pushing on the stack the return
   address and a frame pointer.  When a dynamic alignment is used, some
   architectures have a sizeof(intptr_t) to account for the stack pointer
   which is pushed after the stack alignment.

 - MacroAssemblerSpecific::callWithABI got renamed
   MacroAssembler::callWithABIImpl, as the name is overloaded in the
   MacroAssembler, with a template method which add profiler instrumentation
   around the function call.
Attachment #8637889 - Flags: review?(sstangl)
Attachment #8637889 - Flags: review?(jdemooij)
Attachment #8637889 - Flags: review?(hv1989)
Attachment #8637889 - Flags: review?(branislav.rankov)
Attachment #8637889 - Flags: review?(benj)
Comment on attachment 8637889 [details] [diff] [review]
part 5 - Move callWithABI functions to the generic MacroAssembler.

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

I like it! Can't see anything wrong, and this is mostly code motion, so r=me on the x86 parts. See a suggestion below about the callWithABI vs callWithABIImpl renaming.

::: js/src/jit/MacroAssembler.h
@@ +502,5 @@
> +    void setupAlignedABICall(uint32_t args); // CRASH_ON(arm64)
> +
> +    // Sets up an ABI call for when the alignment is not known. This may need a
> +    // scratch register.
> +    void setupUnalignedABICall(uint32_t args, Register scratch) PER_ARCH;

"args" isn't used by any architecture and could be removed, but as you said, it can be done in a follow-up.

@@ +522,5 @@
> +    // Compute space needed for the function call and set the properties of the
> +    // callee. It returns the space which has to be allocated for calling the
> +    // function.
> +    //
> +    // arg            Number of arguments of the function.

nit: stale comment about "arg"

@@ +529,5 @@
> +    // Reserve the stack and resolve the arguments move.
> +    void callWithABIPre(uint32_t* stackAdjust, bool callFromAsmJS = false) PER_ARCH;
> +
> +    // Emits a call to a C/C++ function, resolving all argument moves.
> +    void callWithABIImpl(void* fun, MoveOp::Type result);

About this renaming: I'd prefer either having "callWithABIImpl" called "callWithABINoProfiler", or having "callWithABI" renamed "callWithABIMaybeProfile" (or something else, showing up the word "profile" somewhere, in some form). The current names don't explicit well the fact that one may activate the profiler and the other won't.

@@ +540,5 @@
> +
> +    // Create the signature to be able to decode the arguments of a native
> +    // function, when calling a function within the simulator.
> +    inline void appendSignatureType(MoveOp::Type type);
> +    inline ABIFunctionType signature() const;

These two could be enclosed in a block #ifdef JS_CODEGEN_RUN_IN_SIMULATOR

::: js/src/jit/x86/MacroAssembler-x86.cpp
@@ +511,5 @@
> +{
> +    uint32_t stackAdjust;
> +    callWithABIPre(&stackAdjust);
> +    call(fun);
> +    callWithABIPost(stackAdjust, result);

This sequence makes me feel like it could also be enclosed in a RAII structure...

@@ +515,5 @@
> +    callWithABIPost(stackAdjust, result);
> +}
> +
> +void
> +MacroAssembler::callWithABIImpl(const Address& fun, MoveOp::Type result)

Maybe you can use templates here?
Attachment #8637889 - Flags: review?(benj) → review+
Comment on attachment 8637889 [details] [diff] [review]
part 5 - Move callWithABI functions to the generic MacroAssembler.

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

Looks fine. ARM64 should be compiling currently on central, though... I would check to make sure that it still does.

::: js/src/jit/MacroAssembler.h
@@ +166,5 @@
> +// assembler to communicate better with the simulator, such that we can safely
> +// cross the boundary between the emulated code and the native code of the host.
> +#if defined(JS_SIMULATOR_ARM) || \
> +    defined(JS_SIMULATOR_ARM64) || \
> +    defined(JS_SIMULATOR_MIPS)

Can just test for JS_SIMULATOR. Don't need JS_CODEGEN_RUN_IN_SIMULATOR at all.

@@ +560,5 @@
> +    // If set by setupUnalignedABICall then callWithABI will pop the stack
> +    // register which is on the stack.
> +    bool dynamicAlignment_;
> +
> +#ifdef JS_CODEGEN_RUN_IN_SIMULATOR

JS_SIMULATOR

::: js/src/jit/arm64/MacroAssembler-arm64.cpp
@@ +601,5 @@
> +
> +void
> +MacroAssembler::callWithABIImpl(Register fun, MoveOp::Type result)
> +{
> +    movePtr(fun, ip0);

While you're here... Instead of hardcoding ip0, this needs to use vixl::UseScratchRegisterScope. That code looks something like:

vixl::UseScratchRegisterScope temps(this);
const Register scratch = temps.AcquireX().asUnsized();

and then use scratch.

@@ +612,5 @@
> +
> +void
> +MacroAssembler::callWithABIImpl(const Address& fun, MoveOp::Type result)
> +{
> +    loadPtr(fun, ip0);

This also needs to use UseScratchRegisterScope instead of hardcoding ip0.

::: js/src/jit/arm64/MacroAssembler-arm64.h
@@ -59,5 @@
>      bool enoughMemory_;
>      uint32_t framePushed_;
>  
> -    // TODO: Can this be moved out of the MacroAssembler and into some shared code?
> -    // TODO: All the code seems to be arch-independent, and it's weird to have this here.

Indeed.
Attachment #8637889 - Flags: review?(sstangl) → review+
Comment on attachment 8637889 [details] [diff] [review]
part 5 - Move callWithABI functions to the generic MacroAssembler.

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

Looks good, no nits other than the ones already mentioned.
Attachment #8637889 - Flags: review?(jdemooij) → review+
Comment on attachment 8637889 [details] [diff] [review]
part 5 - Move callWithABI functions to the generic MacroAssembler.

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

Looks good.

::: js/src/jit/mips/MacroAssembler-mips.cpp
@@ +3624,5 @@
> +
> +void
> +MacroAssembler::callWithABIImpl(Register fun, MoveOp::Type result)
> +{
> +    // Load the callee in t9, as above.

Change places of this comment and the one below it.
Attachment #8637889 - Flags: review?(branislav.rankov) → review+
Comment on attachment 8637880 [details] [diff] [review]
part 4 - MIPS MoveEmitter: Add support for pairs of registers.

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

Looks good.

::: js/src/jit/mips/Assembler-mips.cpp
@@ +43,5 @@
>        case MIRType_Float32:
> +        if (!usedArgSlots_) {
> +            current_ = ABIArg(f12.asSingle());
> +            firstArgFloatSize_ = 1;
> +        } else if (usedArgSlots_ == firstArgFloatSize_) {

Nice solution here.
Attachment #8637880 - Flags: review?(branislav.rankov) → review+
Comment on attachment 8637208 [details] [diff] [review]
part 1 - check_macroassembler_style: Only add an inline prefix in the output, if the methods are inlined.

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

What about?

> if 'inline ' in signature:
>    signature = 'inline ' + re.sub(r'inline\s+', '', signature)
>  	
> signatures =  [
>    { 'arch': a, 'sig': signature }
>    for a in archs
> ]
Attachment #8637208 - Flags: review?(hv1989)
(In reply to Hannes Verschore [:h4writer] from comment #13)
> Comment on attachment 8637208 [details] [diff] [review]
> part 1 - check_macroassembler_style: Only add an inline prefix in the
> output, if the methods are inlined.
> 
> Review of attachment 8637208 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What about?
> 
> > if 'inline ' in signature:
> >    signature = 'inline ' + re.sub(r'inline\s+', '', signature)
> >  	

This would only work for MacroAssembler.h file, but not for all */MacroAssembler*-inl.h files in which the 'inline' keyword is not repeated.  The Inline flag is used to account for both, and the re.sub code is used to remove the 'inline' statement to normalize it in the same manner as any definitions.
Attachment #8637208 - Flags: review?(hv1989)
Blocks: 1190295
Comment on attachment 8637211 [details] [diff] [review]
part 2 - ARM ABIArgGenerator: Add support for soft-fp.

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

Looks good. I also added some suggestions. When not agreeing, please open conversation on irc or in comments.
Else r+.

::: js/src/jit/RegisterSets.h
@@ +1267,5 @@
> +    }
> +    Register gprPair() const {
> +        MOZ_ASSERT(isGeneralRegPair());
> +        return Register::FromCode(u.gpr_ + 1);
> +    }

What about:

Register gpr() const { MOZ_ASSERT(GPR); return ... }
Register evenGpr() const { MOZ_ASSERT(GPR_PAIR); return ... }
Register oddGpr() const { MOZ_ASSERT(GPR_PAIR); return ... }

That way a regpair doesn't get missused as being a single register...
Also I don't like the name of gprPair to signify the second register of the pair...

::: js/src/jit/arm/Architecture-arm.h
@@ +640,5 @@
>  #endif
>  
> +// In order to handle SoftFp ABI calls, we need to be able to express that we
> +// have ABIArg which are representing a pair of general purpose registers, and
> +// also the MoveResolver.

" and also the MoveResolver."

It isn't immediately obvious where the "and" points to. Which parts of the the sentence does it glue together?
Can you rephrase?

@@ +641,5 @@
>  
> +// In order to handle SoftFp ABI calls, we need to be able to express that we
> +// have ABIArg which are representing a pair of general purpose registers, and
> +// also the MoveResolver.
> +#define JS_CODEGEN_HAS_REGISTER_PAIR 1

s/JS_CODEGEN_HAS_REGISTER_PAIR/JS_CODEGEN_REGISTER_PAIR/g

Making it more consistent. We use JS_CODEGEN_ARM_HARDFP instead of JS_CODEGEN_ARM_HAS_HARDFP

::: js/src/jit/arm/Assembler-arm.cpp
@@ +25,4 @@
>  void dbg_break() {}
>  
>  // Note this is used for inter-AsmJS calls and may pass arguments and results in
>  // floating point registers even if the system ABI does not.

Add comment about the use of hard fp by default.
// Also uses hard fp by default.

@@ +59,5 @@
> +        current_ = ABIArg(Register::FromCode(intRegIndex_));
> +        intRegIndex_++;
> +        break;
> +      case MIRType_Double:
> +        // Bump the number of used registers up to the next multiple of two.

We don't bump to the next multiple of two...
0 stays 0 // The text suggest we would bump to 2 in that case
2 stays 2 // the text suggest we would bump to 4 in that case

So better rephrase this as:
// Make sure to use an even register index. Increase to next even number when odd.

@@ +61,5 @@
> +        break;
> +      case MIRType_Double:
> +        // Bump the number of used registers up to the next multiple of two.
> +        intRegIndex_ = (intRegIndex_ + 1) & ~1;
> +        if (intRegIndex_ == NumIntArgRegs) {

Add comment:
// Align stack on 8 bytes.
Attachment #8637211 - Flags: review?(hv1989) → review+
Comment on attachment 8637212 [details] [diff] [review]
part 3 - ARM MoveEmitter: Add support for pairs of registers.

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

In this and the following patch I cannot find where we use MoveOperand with REG_PAIR? Is this dead code, currently?
Else we might want to wait to land this until the actual (follow-up) bug which uses this functionality is ready?

::: js/src/jit/MoveResolver.h
@@ +17,4 @@
>  // This is similar to Operand, but carries more information. We're also not
>  // guaranteed that Operand looks like this on all ISAs.
>  class MoveOperand
>  {

No constructor for REG_PAIR MoveOperand?

@@ +22,5 @@
>      enum Kind {
>          // A register in the "integer", aka "general purpose", class.
>          REG,
> +#ifdef JS_CODEGEN_HAS_REGISTER_PAIR
> +        // A register in the "integer", aka "general purpose", class.

// Two consecutive "integer" register (aka "general purpose"). The even register contains the lower part, the odd register has the high bits of the content.

@@ +92,5 @@
>      }
> +    Register regPair() const {
> +        MOZ_ASSERT(isGeneralRegPair());
> +        return Register::FromCode(code_ + 1);
> +    }

Here also split into:
reg()
evenReg()
oddReg()
?

@@ +121,5 @@
> +        // Check if one of the operand is a registe rpair, in which case, we
> +        // have to check any other register, or register pair.
> +        if (isGeneralRegPair() || other.isGeneralRegPair()) {
> +            if (isGeneralRegPair() && other.isGeneralRegPair()) {
> +                // Pair of registers are contiguous, thus if the first registers

s/contiguous/consequent/ ?

@@ +126,5 @@
> +                // are aliased, then the second registers are aliased too.
> +                MOZ_ASSERT(reg().aliases(other.reg()) == regPair().aliases(other.regPair()));
> +                return reg().aliases(other.reg()) ||
> +                    reg().aliases(other.regPair()) ||
> +                    regPair().aliases(other.reg());

Since the reg pairs need to be aligned. E.g. the evenReg should never alias with the oddReg.

Shouldn't we do:
MOZ_ASSERT(!reg().aliases(other.regPair()));
MOZ_ASSERT(!regPair().aliases(other.reg()));
return reg().aliases(other.reg());

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +4043,5 @@
>          }
>        case MoveOp::FLOAT32:
>          if (!UseHardFpABI()) {
>              // Move float32 from r0 to ReturnFloatReg.
> +            ma_vxfer(r0, ReturnFloat32Reg.singleOverlay());

no need for "singleOverlay()"

::: js/src/jit/arm/MoveEmitter-arm.cpp
@@ +327,5 @@
> +        //
> +        // Warning: if the offset isn't within [-255,+255] then this
> +        // will assert-fail (or, if non-debug, load the wrong words).
> +        // Nothing uses such an offset at the time of this writing.
> +        masm.ma_ldrd(EDtrAddr(src.base, EDtrOffImm(src.offset)), to.reg(), to.regPair());

Can you support this claim? I didn't find the assert place ...
Attachment #8637212 - Flags: review?(hv1989)
Attachment #8637889 - Flags: review?(hv1989) → review+
Attachment #8637208 - Flags: review?(hv1989) → review+
Attachment #8637206 - Flags: review?(hv1989) → review+
(In reply to Hannes Verschore [:h4writer] from comment #16)
> In this and the following patch I cannot find where we use MoveOperand with
> REG_PAIR? Is this dead code, currently?
> Else we might want to wait to land this until the actual (follow-up) bug
> which uses this functionality is ready?

The follow-up patch is the part 5, with the modification in MoveResolver.h, which get the input computed by passABIArg, which forward the result of the part 2, which in case of soft-fp can pass a Double arguments in r0-r1 or r2-r3.

> ::: js/src/jit/MoveResolver.h
> @@ +17,4 @@
> >  // This is similar to Operand, but carries more information. We're also not
> >  // guaranteed that Operand looks like this on all ISAs.
> >  class MoveOperand
> >  {
> 
> No constructor for REG_PAIR MoveOperand?

See the part 5.

> @@ +22,5 @@
> >      enum Kind {
> >          // A register in the "integer", aka "general purpose", class.
> >          REG,
> > +#ifdef JS_CODEGEN_HAS_REGISTER_PAIR
> > +        // A register in the "integer", aka "general purpose", class.
> 
> // Two consecutive "integer" register (aka "general purpose"). The even
> register contains the lower part, the odd register has the high bits of the
> content.

This might not hold on all architectures, and there is no such generic restriction.

> @@ +121,5 @@
> > +        // Check if one of the operand is a registe rpair, in which case, we
> > +        // have to check any other register, or register pair.
> > +        if (isGeneralRegPair() || other.isGeneralRegPair()) {
> > +            if (isGeneralRegPair() && other.isGeneralRegPair()) {
> > +                // Pair of registers are contiguous, thus if the first registers
> 
> s/contiguous/consequent/ ?

Consequent is a false-friend in French (of-importance), I will prefer consecutive.

> @@ +126,5 @@
> > +                // are aliased, then the second registers are aliased too.
> > +                MOZ_ASSERT(reg().aliases(other.reg()) == regPair().aliases(other.regPair()));
> > +                return reg().aliases(other.reg()) ||
> > +                    reg().aliases(other.regPair()) ||
> > +                    regPair().aliases(other.reg());
> 
> Since the reg pairs need to be aligned. E.g. the evenReg should never alias
> with the oddReg.

The fact that they are aligned, sounds like an architecture specific limitation.  In which case fstReg & sndReg might be better than evenReg & oddReg.

The fact that the pair is consecutive is an implementation limitation which is good enough to deal with ARM soft-fp and MIPS ABI, so far.

> ::: js/src/jit/arm/MoveEmitter-arm.cpp
> @@ +327,5 @@
> > +        //
> > +        // Warning: if the offset isn't within [-255,+255] then this
> > +        // will assert-fail (or, if non-debug, load the wrong words).
> > +        // Nothing uses such an offset at the time of this writing.
> > +        masm.ma_ldrd(EDtrAddr(src.base, EDtrOffImm(src.offset)), to.reg(), to.regPair());
> 
> Can you support this claim? I didn't find the assert place ...

The assertion is in the constructor of EDtrOffImm:
https://dxr.mozilla.org/mozilla-central/source/js/src/jit/arm/Assembler-arm.h#852
Attachment #8637212 - Flags: review?(hv1989)
Comment on attachment 8637212 [details] [diff] [review]
part 3 - ARM MoveEmitter: Add support for pairs of registers.

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

Can you update the patch to fix the remaining review comments?

::: js/src/jit/MoveResolver.h
@@ +17,4 @@
>  // This is similar to Operand, but carries more information. We're also not
>  // guaranteed that Operand looks like this on all ISAs.
>  class MoveOperand
>  {

Indeed the constructor with ABIArg as input is used.

@@ +121,5 @@
> +        // Check if one of the operand is a registe rpair, in which case, we
> +        // have to check any other register, or register pair.
> +        if (isGeneralRegPair() || other.isGeneralRegPair()) {
> +            if (isGeneralRegPair() && other.isGeneralRegPair()) {
> +                // Pair of registers are contiguous, thus if the first registers

consecutive is indeed fine.

::: js/src/jit/arm/MoveEmitter-arm.cpp
@@ +327,5 @@
> +        //
> +        // Warning: if the offset isn't within [-255,+255] then this
> +        // will assert-fail (or, if non-debug, load the wrong words).
> +        // Nothing uses such an offset at the time of this writing.
> +        masm.ma_ldrd(EDtrAddr(src.base, EDtrOffImm(src.offset)), to.reg(), to.regPair());

Aha, indeed. It is in EDtrOffImm.
Attachment #8637212 - Flags: review?(hv1989)
Apply review comments.
Attachment #8637212 - Attachment is obsolete: true
Attachment #8646334 - Flags: review?(hv1989)
Comment on attachment 8646334 [details] [diff] [review]
part 3 - ARM MoveEmitter: Add support for pairs of registers.

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

::: js/src/jit/MoveResolver.h
@@ +136,5 @@
> +                return evenReg().aliases(other.evenReg());
> +            } else if (other.isGeneralReg()) {
> +                MOZ_ASSERT(isGeneralRegPair());
> +                return evenReg().aliases(other.reg()) ||
> +                    oddReg().aliases(other.reg());

Nit: can you add three spaces to align evenReg/oddReg

@@ +140,5 @@
> +                    oddReg().aliases(other.reg());
> +            } else if (isGeneralReg()) {
> +                MOZ_ASSERT(other.isGeneralRegPair());
> +                return other.evenReg().aliases(reg()) ||
> +                    other.oddReg().aliases(reg());

Nit: can you add three spaces to align evenReg/oddReg
Attachment #8646334 - Flags: review?(hv1989) → review+
You need to log in before you can comment on or make changes to this bug.