Closed Bug 1127908 Opened 5 years ago Closed 5 years ago

Allow overriding the Ion register allocator with an environment variable

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: bhackett, Assigned: bhackett)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8557161 - Flags: review?(hv1989)
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.
Attachment #8557161 - Flags: review?(hv1989)
Attached patch patchSplinter Review
Assignee: nobody → bhackett1024
Attachment #8557161 - Attachment is obsolete: true
Attachment #8557972 - Flags: review?(hv1989)
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_)
Attachment #8557972 - Flags: review?(hv1989) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/a654baacb8c7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.