Closed Bug 1437862 Opened 2 years ago Closed 2 years ago

Some type barrier clean up

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(4 files)

Some changes to simplify bug 1437483.
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.
Attachment #8950570 - Flags: review?(nicolas.b.pierron)
MacroAssembler::guardTypeSetMightBeIncomplete is only called in debug builds. We can make it #ifdef DEBUG with some work, to make this a bit more explicit.
Attachment #8950578 - Flags: review?(bhackett1024)
Attachment #8950570 - Flags: review?(nicolas.b.pierron) → review+
Priority: -- → P3
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.
Attachment #8952184 - Flags: review?(nicolas.b.pierron)
Attachment #8950578 - Flags: review?(bhackett1024) → review?(nicolas.b.pierron)
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 :)
Attachment #8952184 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8950578 - Flags: review?(nicolas.b.pierron) → review+
(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())
}
Flags: needinfo?(nicolas.b.pierron)
(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.
Flags: needinfo?(nicolas.b.pierron)
(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 :)
Pushed by jandemooij@gmail.com:
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
Keywords: leave-open
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).
Attachment #8952446 - Flags: review?(nicolas.b.pierron)
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?
Attachment #8952446 - Flags: review?(nicolas.b.pierron) → review+
(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).
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1f6650b52d2
part 4 - Ensure guardObjectType has different object and scratch registers. r=nbp
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/d1f6650b52d2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.