When the target is know, we also know the callee types and can see if they match the caller types. So if we know they agree, we can just skip the argument type checks. A quick test shows a 2% on v8, 6% on crypto, 5% on e-b.

Marty, could you let me know if this works on ARM (in theory)? I bind a Label where the second entry point should be (skipping the type arg checks) and during linking I get the second entry point by taking the normal entrypoint and adding the offset of the label.

Comment on attachment 740476 [details] [diff] [review] WIP Review of attachment 740476 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/CodeGenerator.cpp @@ +4654,5 @@ > Linker linker(masm); > IonCode *code = linker.newCode(cx, JSC::ION_CODE); > if (!code) > return false; > + code->setCodeSkippedArgCheck(code->raw() + bodyStart.offset()); I don't think I've ever tested the offset feature of labels. On arm, it will almost certainly return the number of bytes of instructions since the beginning of the buffer, meaning that a call to actualOffset() is needed.

I couldn't do the trick with adding a pointer to JSScript *, like is done for loadBaselineOrIonRaw. Therefore loadBaselineOrIonNoArgCheck gets the right IonScript/BaselineScript separately. I had to combine getting the script and this, to make sure we didn't need an extra register.

(In reply to Hannes Verschore [:h4writer] from comment #3) > I couldn't do the trick with adding a pointer to JSScript *, like is done > for loadBaselineOrIonRaw. Why is this? Bug 858551 nicely simplified the call path and it's unfortunate this bug will add the branching etc back when we can skip the argument check. Ideally the only difference in the generated code is the JSScript offset..

Because we don't have space to add another pointer in "JSScript *". Another solution would be to add the entrypoint in a specific location. I.e. "normal offset - 16" and put a jump there to skip it... And for baseline just reserve some extra space with nops, that would flow into the normal execution ... as in: jump skip_argument_check normal_entry: ; test arguments skip_argument_check: ; body of ioncode Note: I don't think the extra branch has too much overhead. I was thinking the extra complexity wouldn't outweigh the gains ... Another solution would be to add the pointer to "baseline/ion" to baselineScript. Since I think we always have a baselinescript before compiling ionmonkey (in non-eager and non-disabled-baseline execution). Dvander already said doing another loadptr wouldn't be noticeable slower. ...

(In reply to Hannes Verschore [:h4writer] from comment #5) > Because we don't have space to add another pointer in "JSScript *". We could, with fairly little effort, remove JSScript::dataSize again. If you want to add a pointer to JSScript without increasing its size, we should do that.

Specifically, it should be possible to calculate the size along the lines of `atoms - code + natoms * sizeof(atom pointer)`. For details, see the long comment above #define KEEPS_JSVAL_ALIGNMENT(T) in jsscript.h

(In reply to Hannes Verschore [:h4writer] from comment #5) > Because we don't have space to add another pointer in "JSScript *". (In reply to Till Schneidereit [:till] from comment #6) > We could, with fairly little effort, remove JSScript::dataSize again. We can also remove the loop count values now since they were only used by JM+Ion: uint32_t maxLoopCount; uint32_t loopCount;

Comment on attachment 745100 [details] [diff] [review] Patch Review of attachment 745100 [details] [diff] [review]: ----------------------------------------------------------------- Nice! r=me with nits addressed. ::: js/src/ion/CodeGenerator.h @@ +35,5 @@ > class OutOfLineUpdateCache; > > class CodeGenerator : public CodeGeneratorSpecific > { > + Label bodyStart_; Nit: not used ::: js/src/ion/IonBuilder.cpp @@ +4732,5 @@ > return false; > } > > +bool > +TypesMatch(MDefinition *def, types::StackTypeSet *types) Nit: "static bool", and s/TypesMatch/ArgumentTypesMatch and s/types/calleeTypes would be a bit clearer. @@ +4735,5 @@ > +bool > +TypesMatch(MDefinition *def, types::StackTypeSet *types) > +{ > + if (def->resultTypeSet()) { > + if (def->type() != MIRType_Value && !def->mightBeType(def->type())) { Nit: can remove this |if|. @@ +4849,5 @@ > + // If we have a known target, check if the caller arg types are a subset of callee. > + // Since typeset accumulates and can't decrease that means we don't need to check > + // the arguments anymore. > + RootedScript targetScript(cx, target->nonLazyScript()); > + if (targetScript->hasAnalysis()) { We should move this |if| into its own "static bool" function, so that you can "return false;" immediately and avoid checking the other arguments. ::: js/src/ion/MIR.h @@ +1363,5 @@ > CompilerRootScript targetScript_; > // Original value of argc from the bytecode. > uint32_t numActualArgs_; > > + bool argCheck_; Nit: s/argCheck_/needsArgCheck_. ::: js/src/jsscript.h @@ +410,5 @@ > private: > uint32_t useCount; /* Number of times the script has been called > * or has had backedges taken. Reset if the > * script's JIT code is forcibly discarded. */ > + uint32_t PADDING32; Does the compiler complain if you remove the padding? If not, it's best to remove it, if we only need it on 32/64 bit you should wrap it in "#if JS_BITS_PER_WORD == 32/64".