989759 - Add a tag to identify serialized ResumePoint in RecoverBuffer.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

As we are planning to add different kind of RInstruction, we need to be able to dispatch between them.  The goal is to lose the knowledge that all frames are RResumePoint, and leave this knowledge inside the RecoverBuffer.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: [part 1] Identify every ResumePoint RInstruction.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: [part 2] Dispatch based on instruction identifier.

This patch add the main logic to dispatch to different kind of RInstructions and to identify them.  The boiler-plate code is made to look exactly like the code generated by Macros in MIR.h

I did not introduced the Macro yet, as they are not as readable and that this is not necessary for one instruction.  I will add the Macro as soon as I add an optimization which make use of these paths.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8399130 [details] [diff] [review]
[part 1] Identify every ResumePoint RInstruction.

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

::: js/src/jit/Recover.cpp
@@ +95,5 @@
>  void
>  RResumePoint::readRecoverData(CompactBufferReader &reader, Storage *raw)
>  {
> +    mozilla::DebugOnly<uint32_t> op = reader.readUnsigned();
> +    MOZ_ASSERT(op == uint32_t(Recover_ResumePoint));

Not for this patch, but once we add more instructions we should add Recover_Limit so that we can assert op < Recover_Limit :)

::: js/src/jit/Recover.h
@@ +17,5 @@
>  typedef mozilla::AlignedStorage<MAX_RINSTRUCTION_SIZE> Storage;
>  
> +enum RecoverOpcode
> +{
> +    Recover_ResumePoint

Recover_ResumePoint = 0 -- MSVC has some weird (signed) enum quirks, so it'd be good to specify 0 for the first item just to be safe.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8399133 [details] [diff] [review]
[part 2] Dispatch based on instruction identifier.

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

As mentioned below we should add macros to avoid defining the same set of methods for each instruction manually, but doing that as a separate patch is fine.

::: js/src/jit/Recover.cpp
@@ +14,5 @@
> +RInstruction::readRecoverData(CompactBufferReader &reader, Storage *raw)
> +{
> +    uint32_t op = reader.readUnsigned();
> +    switch (Opcode(op))
> +    {

Nit: { on previous line.

::: js/src/jit/Recover.h
@@ +49,5 @@
>      RResumePoint(CompactBufferReader &reader);
>  
>    public:
> +    virtual Opcode opcode() const {
> +        return Recover_ResumePoint;

In a separate patch we should add a macro like we have for LIR and MIR, something like:

    RINS_HEADER(ResumePoint)

Same for the isResumePoint/toResumePoint methods.

@@ +61,5 @@
>      }
>  };
>  
> +const RResumePoint *
> +RInstruction::toResumePoint() const {

Nit: { on its own line.

Comment 5

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee9fbb00aa80
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dcbb0f401d8

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G bustage in the push.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4d1adee6057

https://tbpl.mozilla.org/php/getParsedLog.php?id=37290188&tree=Mozilla-Inbound

Comment 7

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebcdd5ea79ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c4295f019eb

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ebcdd5ea79ab
https://hg.mozilla.org/mozilla-central/rev/0c4295f019eb