1437862 - Some type barrier clean up

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Jan de Mooij [:jandem]]

Some changes to simplify bug 1437483.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - generateArgumentsChecks registers

In CodeGenerator::generateArgumentsChecks, we remove OsrFrameReg from the set but that doesn't make sense since the OSR entry is a completely separate entry point.

Furthermore, all registers should be available there so let's just use GeneralRegisterSet::All() and then we can remove Registers::TempMask.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - guardTypeSetMightBeIncomplete changes

MacroAssembler::guardTypeSetMightBeIncomplete is only called in debug builds. We can make it #ifdef DEBUG with some work, to make this a bit more explicit.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Refactor guardTypeSet / guardObjectType

We currently use the BranchType/BranchGCPtr abstraction, but I find this pretty hard to reason about.

I think it's easier to use an explicit counter to check if we're emitting the last branch, and then we can just invert the condition manually.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8952184 [details] [diff] [review]
Part 3 - Refactor guardTypeSet / guardObjectType

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

follow-up: Do the same with CodeGenerator::visitObjectGroupDispatch :)

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8950578 [details] [diff] [review]
Part 2 - guardTypeSetMightBeIncomplete changes

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

r=me with the following comment fixed.

::: js/src/jit/CodeGenerator.cpp
@@ +4954,5 @@
>      if (miss.used()) {
>          if (bailout) {
>              bailoutFrom(&miss, graph.entrySnapshot());
>          } else {
> +#ifdef DEBUG

This code is completely miss-leading to me. From what I understand we always bailout except when we are asserting that we have the proper allocations.

I will suggest changing the argument name from "bailout == true" to "assert == false", move the "if (bailout)" body after this code, such as it looks like:

  if (miss.used()) {
#ifdef DEBUG
      if (assert) {
          // …
      }
#endif

      bailoutFrom(&miss, graph.entrySnapshot());
  }


This way I would be confident that we do generate a bailout if we do not debug (even if it never happens), and always bind the miss label if ever used, and that without knowing anything about the callers.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> I will suggest changing the argument name from "bailout == true" to "assert
> == false", move the "if (bailout)" body after this code, such as it looks
> like:

This doesn't work because we can't bind the |miss| label twice.

I can go with

if (assert) {
#ifdef DEBUG
    ...
#else
    MOZ_CRASH..
#endif
} else {
    bailoutFrom(&miss, graph.entrySnapshot())
}

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> (In reply to Nicolas B. Pierron [:nbp] from comment #5)
> > I will suggest changing the argument name from "bailout == true" to "assert
> > == false", move the "if (bailout)" body after this code, such as it looks
> > like:
> 
> This doesn't work because we can't bind the |miss| label twice.
> 
> I can go with
> 
> if (assert) {
> #ifdef DEBUG
>     ...
> #else
>     MOZ_CRASH..
> #endif
> } else {
>     bailoutFrom(&miss, graph.entrySnapshot())
> }

Sounds fine, thanks.

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] {backlog: ~41} from comment #4)
> follow-up: Do the same with CodeGenerator::visitObjectGroupDispatch :)

Yeah I'll get to it when looking for object type guards to fix :)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fac52eb6527
part 1 - Clean up register allocation in generateArgumentsChecks. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3fd4aff7c79
part 2 - Make guardTypeSetMightBeIncomplete debug-only. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fbb23dba529
part 3 - Refactor guardTypeSet and guardObjectType to be a bit simpler. r=nbp

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Fix scratch registers

In MacroAssembler::guardObjectType, it's possible to have obj == scratch. This is pretty annoying so the patch fixes it by adding an extra scratch register to guardTypeSet (but only if we need it for object guards).

Comment 11

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/6fac52eb6527
https://hg.mozilla.org/mozilla-central/rev/d3fd4aff7c79
https://hg.mozilla.org/mozilla-central/rev/4fbb23dba529

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8952446 [details] [diff] [review]
Part 4 - Fix scratch registers

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

::: js/src/jit/IonCacheIRCompiler.cpp
@@ +1451,5 @@
> +                regs.take(valReg.typedReg().gpr());
> +        }
> +        regs.take(scratch1);
> +        objScratch = regs.takeAny();
> +        masm.Push(objScratch);

How reliable is the liveRegs argument? Could we use it to find unused allocatable registers here and above?

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] {backlog: ~41} from comment #12)
> How reliable is the liveRegs argument? Could we use it to find unused
> allocatable registers here and above?

We could but the CacheIR allocator may have used some of these registers too (we only use liveRegs for the FP registers here).

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1f6650b52d2
part 4 - Ensure guardObjectType has different object and scratch registers. r=nbp

Comment 15

[Andrei Ciure[:aciure]]

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