Closed
Bug 989759
Opened 11 years ago
Closed 11 years ago
Add a tag to identify serialized ResumePoint in RecoverBuffer.
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: nbp, Assigned: nbp)
References
Details
Attachments
(2 files)
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•11 years ago
|
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #8399130 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8399133 -
Attachment description: [part 2] Dispatch based instruction identifier. → [part 2] Dispatch based on instruction identifier.
Comment 3•11 years ago
|
||
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 4•11 years ago
|
||
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+
Assignee | ||
Comment 5•11 years ago
|
||
Comment 6•11 years ago
|
||
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
Assignee | ||
Comment 7•11 years ago
|
||
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ebcdd5ea79ab
https://hg.mozilla.org/mozilla-central/rev/0c4295f019eb
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•