Closed Bug 989759 Opened 6 years ago Closed 5 years ago

Add a tag to identify serialized ResumePoint in RecoverBuffer.

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(2 files)

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.
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
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.
Attachment #8399133 - Flags: review?(jdemooij)
Attachment #8399133 - Attachment description: [part 2] Dispatch based instruction identifier. → [part 2] Dispatch based on instruction identifier.
Blocks: 989930
Blocks: 990106
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.
Attachment #8399130 - Flags: review?(jdemooij) → review+
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.
Attachment #8399133 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/ebcdd5ea79ab
https://hg.mozilla.org/mozilla-central/rev/0c4295f019eb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.