1127908 - Allow overriding the Ion register allocator with an environment variable

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Currently the default register allocator can only be changed in the shell.  Using an environment variable in addition to the shell option lets the browser change the allocator used and allows for easier integration with AWFY test harnesses than a browser config option.

Comment 1

[Hannes Verschore [:h4writer]]

Comment on attachment 8557161 [details] [diff] [review]
patch

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

There is already a way to override the JitOptions using env. variable.
But the register allocator hasn't been patched to use that.
https://dxr.mozilla.org/mozilla-central/source/js/src/jit/JitOptions.cpp?from=js/src/jit/JitOptions.cpp&case=true#104

Can you update your code to read the env. there? That is better than checking it every compilation.
So when the "JIT_OPTION_forcedRegisterAllocator" env is set, put it in "forcedRegisterAllocator" and set forceRegisterAllocator to true.


Bonuspoints (so not required) if you adjust "forcedDefaultIonWarmUpThreshold" to do the same.
If the env is set, put the result in "forcedDefaultIonWarmUpThreshold" and set "forceDefaultIonWarmUpThreshold" to true.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 8557972 [details] [diff] [review]
patch

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

Thanks!

::: js/src/jit/IonOptimizationLevels.cpp
@@ +35,5 @@
>      autoTruncate_ = true;
>      sink_ = true;
>      registerAllocator_ = RegisterAllocator_LSRA;
>  
> +    inlineMaxTotalBytecodeLength_ = CompilerWarmupThreshold;

Think you made a fault here. This should stay and you should do this for "compilerWarmUpThreshold_" (instead of inlineMaxTotalBytecodeLength_)

Comment 4

[Brian Hackett [Laid off!]]

Oops, good catch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b393c8dae2b

Comment 5

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6212514&repo=mozilla-inbound

Comment 6

[Hannes Verschore [:h4writer]]

Comment on attachment 8557972 [details] [diff] [review]
patch

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

I think that is the issue why it got backed out.

::: js/src/jit/JitOptions.cpp
@@ +172,5 @@
>  JitOptions::setEagerCompilation()
>  {
>      eagerCompilation = true;
>      baselineWarmUpThreshold = 0;
> +    forcedDefaultIonWarmUpThreshold.emplace(0);

seems like you need to call "reset" first, before you can do emplace.

@@ +178,5 @@
>  
>  void
>  JitOptions::setCompilerWarmUpThreshold(uint32_t warmUpThreshold)
>  {
> +    forcedDefaultIonWarmUpThreshold.emplace(warmUpThreshold);

seems like you need to call "reset" first, before you can do emplace.

Comment 7

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a654baacb8c7

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a654baacb8c7