The default bug view has changed. See this FAQ.

IonMonkey: Support adding non-effectful recover instruction

RESOLVED FIXED in mozilla32

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

(Blocks: 1 bug, {feature})

unspecified
mozilla32
feature
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
This bug would be separated in 3 main parts:
 - Adding a way to add any MInstruction into the vector of the LRecoverInfo, and encode the snapshot correctly.
 - Adding a way to evaluate any RInstruction from BaselineBailout.
 - (tiny use-case) Instrument DCE to remove some arithmetic operations.
(Assignee)

Comment 1

3 years ago
Created attachment 8400903 [details] [diff] [review]
[part 1] RecoverWriter support MNode as instructions.
Attachment #8400903 - Flags: review?(jdemooij)
(Assignee)

Updated

3 years ago
Attachment #8400903 - Attachment description: RecoverWriter support MNode as instructions. → [part 1] RecoverWriter support MNode as instructions.
(Assignee)

Comment 2

3 years ago
Created attachment 8400906 [details] [diff] [review]
[part 2] LRecoverInfo consider MDefinition flagged as Recovered.

This patch does a topological sort of the recover instructions when lowering the resume points.

Also, this patch add a way to iterate over all the operands of the recover instructions, which is especially useful for filling the LSnapshot with the list of LAllocations.
Attachment #8400906 - Flags: review?(hv1989)
(Assignee)

Comment 3

3 years ago
Created attachment 8400911 [details] [diff] [review]
[part 3] Handle RecoverInstruction in bailouts

Add support for encoding/decoding RInstructions in Snapshots, as well as recovering (by executing) RInstructions during the bailout to baseline.

This patch also add Macro to extend as needed the list of RInstructions.

Note: There is no need to add other isFoo & toFoo methods, as long as we do not specialize the recovering path in BaselineBailout.cpp (as we currently do to support ResumePoint)
Attachment #8400911 - Flags: review?(jdemooij)
(Assignee)

Comment 4

3 years ago
Created attachment 8400913 [details] [diff] [review]
[part 4] DCE Additions captured by resume points.

This patch highlight how to implement one non-effectful RInstruction, and how to instrument DCE to make use of this RInstruction.
Attachment #8400913 - Flags: review?(jdemooij)
Attachment #8400913 - Flags: review?(hv1989)
Attachment #8400913 - Flags: feedback?(kvijayan)
Attachment #8400913 - Flags: feedback?(efaustbmo)
Comment on attachment 8400913 [details] [diff] [review]
[part 4] DCE Additions captured by resume points.

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

In "ins->setRecovered()" can you assert the following. This is the rule used by LICM to know when it can hoist instructions. Since recover instruction have the same constraints on what it should enable to recover, it might be good to assert on that.

ins->isMovable() && !ins->isEffectful() && !ins->neverHoist();

::: js/src/jit/MIR.h
@@ +3946,5 @@
>      bool isOperandTruncated(size_t index) const;
> +
> +    bool writeRecoverData(CompactBufferWriter &writer) const;
> +    bool isRecoverable() const {
> +        return specialization_ == MIRType_Int32;

I think you can adjust this to specialization_ < MIRType_Object.

::: js/src/jit/Recover.cpp
@@ +136,5 @@
> +
> +RAdd::RAdd(CompactBufferReader &reader)
> +{
> +    static_assert(sizeof(*this) <= sizeof(RInstructionStorage),
> +                  "Storage space is too small to decode this recover instruction.");

I don't really understand this assert? We only save an enum in the RecoverBuffer right?

@@ +145,5 @@
> +{
> +    RootedValue lhs(cx, iter.read());
> +    RootedValue rhs(cx, iter.read());
> +    RootedValue result(cx);
> +

JS_ASSERT(!lhs.isObject());
JS_ASSERT(!rhs.isObject());

::: js/src/jit/Recover.h
@@ +91,5 @@
> +    RINSTRUCTION_HEADER_(Add)
> +
> +    virtual uint32_t numOperands() const {
> +        return 2;
> +    }

This feels a bit strange. I think that the numOperands is defined by the operands of MAdd? So why do we need it here? Also I don't see where it is used?
Attachment #8400913 - Flags: review?(hv1989)
(Assignee)

Comment 6

3 years ago
(In reply to Hannes Verschore [:h4writer] from comment #5)
> In "ins->setRecovered()" can you assert the following.

No, because it is defined by the Macro that we use for all MIR flags.

> This is the rule used
> by LICM to know when it can hoist instructions. Since recover instruction
> have the same constraints on what it should enable to recover, it might be
> good to assert on that.
> 
> ins->isMovable() && !ins->isEffectful() && !ins->neverHoist();

Recover instructions do not have the same constraints.  I do not see any need to check for these at the moment as DCE is done last, after GVN and LICM.

> ::: js/src/jit/MIR.h
> @@ +3946,5 @@
> >      bool isOperandTruncated(size_t index) const;
> > +
> > +    bool writeRecoverData(CompactBufferWriter &writer) const;
> > +    bool isRecoverable() const {
> > +        return specialization_ == MIRType_Int32;
> 
> I think you can adjust this to specialization_ < MIRType_Object.

I could, but I did not want to support Float32 specialization for the moment, because this would imply that we are saving an extra RAddFloat32 or that we keep an extra flag, which is not the concern of this optimization.

> ::: js/src/jit/Recover.cpp
> @@ +136,5 @@
> > +
> > +RAdd::RAdd(CompactBufferReader &reader)
> > +{
> > +    static_assert(sizeof(*this) <= sizeof(RInstructionStorage),
> > +                  "Storage space is too small to decode this recover instruction.");
> 
> I don't really understand this assert? We only save an enum in the
> RecoverBuffer right?

This is something which should be present in all RInstruction constructors, as these are copy & paste mechanical lines which ensure that decoding an RAdd instruction will always fit in the RInstructionStorage space reserved for decoding the current RInstruction.

> ::: js/src/jit/Recover.h
> @@ +91,5 @@
> > +    RINSTRUCTION_HEADER_(Add)
> > +
> > +    virtual uint32_t numOperands() const {
> > +        return 2;
> > +    }
> 
> This feels a bit strange. I think that the numOperands is defined by the
> operands of MAdd? So why do we need it here? Also I don't see where it is
> used?

We need it here because we do not yet have enough RInstructions to justify inheriting from RBinaryInstruction.
And also because this is needed to know the number of allocations we are expected to read from the snapshot.
(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> (In reply to Hannes Verschore [:h4writer] from comment #5)
> > In "ins->setRecovered()" can you assert the following.
> 
> No, because it is defined by the Macro that we use for all MIR flags.

Sure, in that case you should do it when we are iterating the MIR instructions to create recover instructions out of them.

> > This is the rule used
> > by LICM to know when it can hoist instructions. Since recover instruction
> > have the same constraints on what it should enable to recover, it might be
> > good to assert on that.
> > 
> > ins->isMovable() && !ins->isEffectful() && !ins->neverHoist();
> 
> Recover instructions do not have the same constraints.  I do not see any
> need to check for these at the moment as DCE is done last, after GVN and
> LICM.

This line has nothing to do with when it happens (before or after GVN/LICM). This is to make sure we don't mark instructions that can't be moved or are effectfull as recoverable. (Maybe the neverHoist part isn't needed). Since a recover instruction will be run out of order, we shouldn't do this with such instructions!

> > ::: js/src/jit/MIR.h
> > @@ +3946,5 @@
> > >      bool isOperandTruncated(size_t index) const;
> > > +
> > > +    bool writeRecoverData(CompactBufferWriter &writer) const;
> > > +    bool isRecoverable() const {
> > > +        return specialization_ == MIRType_Int32;
> > 
> > I think you can adjust this to specialization_ < MIRType_Object.
> 
> I could, but I did not want to support Float32 specialization for the
> moment, because this would imply that we are saving an extra RAddFloat32 or
> that we keep an extra flag, which is not the concern of this optimization.

We would just do a double addition in that case, right? I don't see a reason this would be bad. Only IonMonkey knows about floats. So if we go back to baseline it doesn't matter that we were working floats, right?

> > ::: js/src/jit/Recover.cpp
> > @@ +136,5 @@
> > > +
> > > +RAdd::RAdd(CompactBufferReader &reader)
> > > +{
> > > +    static_assert(sizeof(*this) <= sizeof(RInstructionStorage),
> > > +                  "Storage space is too small to decode this recover instruction.");
> > 
> > I don't really understand this assert? We only save an enum in the
> > RecoverBuffer right?
> 
> This is something which should be present in all RInstruction constructors,
> as these are copy & paste mechanical lines which ensure that decoding an
> RAdd instruction will always fit in the RInstructionStorage space reserved
> for decoding the current RInstruction.

No macro foo here?
Comment on attachment 8400906 [details] [diff] [review]
[part 2] LRecoverInfo consider MDefinition flagged as Recovered.

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

::: js/src/jit/LIR.cpp
@@ +146,5 @@
>  bool
> +LRecoverInfo::appendOperands(MNode *ins)
> +{
> +    size_t e = ins->numOperands();
> +    for (size_t i = 0; i < e; i++) {

you can put the ins->numOperands(); inside the for?

::: js/src/jit/LIR.h
@@ +878,5 @@
>  
>  class LRecoverInfo : public TempObject
>  {
>    public:
> +    typedef Vector<MNode *, 0, IonAllocPolicy> Instructions;

No default space? What would be a good default?

@@ +922,5 @@
> +    size_t numInstructions() const {
> +        return instructions_.length();
> +    }
> +
> +    class OperandIter

Add a comment what this does.

::: js/src/jit/MIR.h
@@ +79,5 @@
> +                                                                                \
> +    /* Marks if the current instruction should go to the bailout paths instead
> +     * of producing code as part of the control flow.  This flag can only be set
> +     * on instructions which are only used by ResumePoint or by other flagged
> +     * instructions.

* other instructions flagged as recovered

=> can only be set on instructions which are only used by ResumePoint ...
- Do we enforce this variant? If not we should!

@@ +81,5 @@
> +     * of producing code as part of the control flow.  This flag can only be set
> +     * on instructions which are only used by ResumePoint or by other flagged
> +     * instructions.
> +     */                                                                         \
> +    _(Recovered)

Wouldn't it make make sense to call it xxRecoverInstruction instead of xxRecovered?
isRecoverInstruction/setRecoverInstruction

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ -140,4 @@
>                                         uint32_t *startIndex)
>  {
> -    IonSpew(IonSpew_Codegen, "Encoding %u of resume point %p's operands starting from %u",
> -            resumePoint->numOperands(), (void *) resumePoint, *startIndex);

I think we loose this spewing. I don't see it somewhere else? Can you add it again?
Attachment #8400906 - Flags: review?(hv1989) → review+
(Assignee)

Updated

3 years ago
Blocks: 991720
(Assignee)

Comment 9

3 years ago
(In reply to Hannes Verschore [:h4writer] from comment #8)
> Comment on attachment 8400906 [details] [diff] [review]
> [part 2] LRecoverInfo consider MDefinition flagged as Recovered.
> 
> Review of attachment 8400906 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/LIR.cpp
> @@ +146,5 @@
> >  bool
> > +LRecoverInfo::appendOperands(MNode *ins)
> > +{
> > +    size_t e = ins->numOperands();
> > +    for (size_t i = 0; i < e; i++) {
> 
> you can put the ins->numOperands(); inside the for?

numOperands is a virtual function call.  Sunfish recently changed all of them such as we read the length as part if the init of the for loop.  Is that what you mean, or you want it as part of the condition?

> ::: js/src/jit/LIR.h
> @@ +878,5 @@
> >  
> >  class LRecoverInfo : public TempObject
> >  {
> >    public:
> > +    typedef Vector<MNode *, 0, IonAllocPolicy> Instructions;
> 
> No default space? What would be a good default?

Jandem already addressed this point in Bug 989641, I just did not rebased on top of his patch before attaching this one on Bugzilla, now this is 2 instead of 0.

> ::: js/src/jit/MIR.h
> @@ +79,5 @@
> > +                                                                                \
> > +    /* Marks if the current instruction should go to the bailout paths instead
> > +     * of producing code as part of the control flow.  This flag can only be set
> > +     * on instructions which are only used by ResumePoint or by other flagged
> > +     * instructions.
> 
> * other instructions flagged as recovered
> 
> => can only be set on instructions which are only used by ResumePoint ...
> - Do we enforce this variant? If not we should!

True, I will make a part 5 to assert the MIR graph coherence.

> @@ +81,5 @@
> > +     * of producing code as part of the control flow.  This flag can only be set
> > +     * on instructions which are only used by ResumePoint or by other flagged
> > +     * instructions.
> > +     */                                                                         \
> > +    _(Recovered)
> 
> Wouldn't it make make sense to call it xxRecoverInstruction instead of
> xxRecovered?
> isRecoverInstruction/setRecoverInstruction

I am open to suggestions, I cannot settle on any good short name for it.

  isRecovered, setRecovered
  isRecoverInstruction, setRecoverInstruction (set as Recover Instruction ?)
  isRInstruction, setRInstruction (set as RInstruction ?)

maybe

  isOnBailout, setOnBailout
  isOnBailoutPath, setOnBailoutPath

> ::: js/src/jit/shared/CodeGenerator-shared.cpp
> @@ -140,4 @@
> >                                         uint32_t *startIndex)
> >  {
> > -    IonSpew(IonSpew_Codegen, "Encoding %u of resume point %p's operands starting from %u",
> > -            resumePoint->numOperands(), (void *) resumePoint, *startIndex);
> 
> I think we loose this spewing. I don't see it somewhere else? Can you add it
> again?

Yes, we do loose this one, if we really want it back then we would need a function to know when we hit the first operand of an instruction.  I can look on adding something similar which prints the MIR Instruction number as well as the resume points.
(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> > you can put the ins->numOperands(); inside the for?
> 
> numOperands is a virtual function call.  Sunfish recently changed all of
> them such as we read the length as part if the init of the for loop.  Is
> that what you mean, or you want it as part of the condition?
> 

Ok this is for performance. In that case just rename e to length or size or something along. E is not really descriptive.

> > @@ +81,5 @@
> > > +     * of producing code as part of the control flow.  This flag can only be set
> > > +     * on instructions which are only used by ResumePoint or by other flagged
> > > +     * instructions.
> > > +     */                                                                         \
> > > +    _(Recovered)
> > 
> > Wouldn't it make make sense to call it xxRecoverInstruction instead of
> > xxRecovered?
> > isRecoverInstruction/setRecoverInstruction
> 
> I am open to suggestions, I cannot settle on any good short name for it.
> 
>   isRecovered, setRecovered
>   isRecoverInstruction, setRecoverInstruction (set as Recover Instruction ?)
>   isRInstruction, setRInstruction (set as RInstruction ?)
> 
> maybe
> 
>   isOnBailout, setOnBailout
>   isOnBailoutPath, setOnBailoutPath

Needinfo: jan? (Or anybody else that has an opinion).
I would vote for "RecoverInstruction" currently. But I think before I totally understood this, I would have expected "OnBailout". But I really wouldn't take "Recovered". When iterating over the MIR and doing isRecovered() it looks like it already happened. Which isn't true...
Flags: needinfo?(jdemooij)
(Assignee)

Comment 11

3 years ago
Created attachment 8401393 [details] [diff] [review]
[part 5] Assert lack of live uses of recover instructions.
Attachment #8401393 - Flags: review?(hv1989)
Comment on attachment 8401393 [details] [diff] [review]
[part 5] Assert lack of live uses of recover instructions.

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

Thanks!

::: js/src/jit/IonAnalysis.cpp
@@ +1314,5 @@
>          for (MDefinitionIterator iter(*block); iter; iter++) {
>              JS_ASSERT(iter->block() == *block);
> +
> +            // Assert that use chains are valid for this instruction.
> +            for (uint32_t i = 0, e = iter->numOperands(); i < e; i++)

can you also rename the "e" to "size" or "numOperands" or something
Attachment #8401393 - Flags: review?(hv1989) → review+
(Assignee)

Comment 13

3 years ago
Comment on attachment 8400906 [details] [diff] [review]
[part 2] LRecoverInfo consider MDefinition flagged as Recovered.

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

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +299,5 @@
>      }
>  
> +    DebugOnly<uint32_t> numRecoverInst =
> +        recoverInfo->numInstructions() - recoverInfo->mir()->frameCount();
> +    MOZ_ASSERT(snapshot->numSlots() + numRecoverInst == snapshots_.allocWritten());

This assertion does not work if the same recovered instruction is used multiple times.  I have a test case which make this assertion fail.  In the mean time I've replace this assertion by

  MOZ_ASSERT(snapshot->numSlots() == startIndex);

checked that every call to encodeAllocation is increasing snapshots_.allocWritten() by 1.
and I renamed startIndex to allocIndex (as well as in encodeAllocation).
(Assignee)

Updated

3 years ago
Blocks: 992845
Comment on attachment 8400903 [details] [diff] [review]
[part 1] RecoverWriter support MNode as instructions.

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

Apologies for the delay. Finally getting my review/needinfo queues a bit under control again.

::: js/src/jit/Recover.cpp
@@ +11,5 @@
>  
> +bool
> +MNode::writeRecoverData(CompactBufferWriter &writer) const
> +{
> +    MOZ_ASSUME_UNREACHABLE("This instruction cannot be serializable?");

Nit: "cannot be serialized" or "is not serializable"
Attachment #8400903 - Flags: review?(jdemooij) → review+
(In reply to Hannes Verschore [:h4writer] from comment #10)
> > I am open to suggestions, I cannot settle on any good short name for it.
> > 
> >   isRecovered, setRecovered
> >   isRecoverInstruction, setRecoverInstruction (set as Recover Instruction ?)
> >   isRInstruction, setRInstruction (set as RInstruction ?)
> > 
> > maybe
> > 
> >   isOnBailout, setOnBailout
> >   isOnBailoutPath, setOnBailoutPath
> 
> Needinfo: jan? (Or anybody else that has an opinion).
> I would vote for "RecoverInstruction" currently.

isRecoverInstruction/setRecoverInstruction seemed nice at first, but it's also confusing because the term "recover instruction" is used for the R* instruction (and this is about the MIR instruction).

So, considering that, OnBailout(Path) seems less confusing...
Flags: needinfo?(jdemooij)
Comment on attachment 8400911 [details] [diff] [review]
[part 3] Handle RecoverInstruction in bailouts

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

Very nice. r=me with comments addressed (mostly comment nits).

::: js/src/jit/IonFrameIterator.h
@@ +255,4 @@
>      IonJSFrameLayout *fp_;
>      MachineState machine_;
>      IonScript *ionScript_;
> +    AutoValueVector *instructionsResults_;

Nit: instructionResults_ reads better.

@@ +343,5 @@
>  
> +    // Register a vector used for storing the results of the evaluation of
> +    // recover instructions. This vector should be registered before the
> +    // beginning of the iteration. This function is in charge of allocating
> +    // enough space of recovering all instructions results, and return false iff

"enough space for all instruction results, and..."

@@ +345,5 @@
> +    // recover instructions. This vector should be registered before the
> +    // beginning of the iteration. This function is in charge of allocating
> +    // enough space of recovering all instructions results, and return false iff
> +    // it fails.
> +    bool initIntructionsResults(AutoValueVector &results);

Nit: initInstructionResults

::: js/src/jit/IonFrames.cpp
@@ +1540,5 @@
> +SnapshotIterator::initIntructionsResults(AutoValueVector &results)
> +{
> +    MOZ_ASSERT(recover_.numInstructionsRead() == 1);
> +
> +    // The last instruction will always be a resume point, no need to allocate a

Nit: s/allocate a/allocate

::: js/src/jit/Recover.cpp
@@ +123,5 @@
> +
> +bool
> +RResumePoint::recover(JSContext *cx, SnapshotIterator &iter) const
> +{
> +    MOZ_ASSUME_UNREACHABLE("This instruction cannot be serializable?");

Nit: see review comment on previous patch, "serialized" or "is not serializable"

::: js/src/jit/Recover.h
@@ +34,5 @@
>  
>      virtual Opcode opcode() const = 0;
>  
> +    // As Opposed to the MIR, there is no need to add more execption as every
> +    // other instruction is well abstracted under the "recover" function.

Nit: s/Opposed/opposed, s/execption/exception. Or maybe "methods"

@@ +40,5 @@
>          return opcode() == Recover_ResumePoint;
>      }
>      inline const RResumePoint *toResumePoint() const;
>  
> +    // Number of allocation which are encoded in the Snapshot for recovering the

Nit: allocations

@@ +46,5 @@
>      virtual uint32_t numOperands() const = 0;
>  
> +    // Function used to recover the value computed by this instruction. This
> +    // functions reads its arguments from the allocations listed on the snapshot
> +    // iterator and store its returned value on the snapshot iterator too.

Nit: s/functions/function, s/store/stores
Attachment #8400911 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #15)
> isRecoverInstruction/setRecoverInstruction seemed nice at first, but it's
> also confusing because the term "recover instruction" is used for the R*
> instruction (and this is about the MIR instruction).
> 
> So, considering that, OnBailout(Path) seems less confusing...

I just noticed that part 4 uses isRecoverable/setRecoverable. Using OnBailout(Path) is not a good name then, unless we rename Recoverable too; they should have names that sound similar.
Comment on attachment 8400913 [details] [diff] [review]
[part 4] DCE Additions captured by resume points.

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

::: js/src/jit/MIR.h
@@ +3946,5 @@
>      bool isOperandTruncated(size_t index) const;
> +
> +    bool writeRecoverData(CompactBufferWriter &writer) const;
> +    bool isRecoverable() const {
> +        return specialization_ == MIRType_Int32;

If you do what Hannes suggested, please add the following too:

MOZ_ASSERT_IF(specialization_ < MIRType_Object, !isEffectful());

::: js/src/jit/Recover.cpp
@@ +127,5 @@
>      MOZ_ASSUME_UNREACHABLE("This instruction cannot be serializable?");
>  }
> +
> +bool
> +MAdd::writeRecoverData(CompactBufferWriter &writer) const

Could the caller of writeRecoverData assert def->isRecoverable() and def->type() != MIRType_None etc?

@@ +135,5 @@
> +}
> +
> +RAdd::RAdd(CompactBufferReader &reader)
> +{
> +    static_assert(sizeof(*this) <= sizeof(RInstructionStorage),

Just wondering, could we move this assert somewhere else, so that it's automatically checked for all instructions? Maybe with some template or macro magic? Doesn't matter too much; just would be nice if possible.
Attachment #8400913 - Flags: review?(jdemooij)
(Assignee)

Comment 19

3 years ago
(In reply to Jan de Mooij [:jandem] from comment #18)
> @@ +135,5 @@
> > +}
> > +
> > +RAdd::RAdd(CompactBufferReader &reader)
> > +{
> > +    static_assert(sizeof(*this) <= sizeof(RInstructionStorage),
> 
> Just wondering, could we move this assert somewhere else, so that it's
> automatically checked for all instructions? […]

I will add this as part of part 3:

+#   define MATCH_OPCODES_(op)                                           \
+      case Recover_##op:                                                \
+        static_assert(sizeof(R##op) <= sizeof(RInstructionStorage),     \
+                      "Storage space is too small to decode R" #op " instructions."); \
+        new (raw->addr()) R##op(reader);

> […] Maybe with some template or
> macro magic? […]

I don't see what you are talking about :D
(Assignee)

Comment 20

3 years ago
(In reply to Jan de Mooij [:jandem] from comment #17)
> (In reply to Jan de Mooij [:jandem] from comment #15)
> > isRecoverInstruction/setRecoverInstruction seemed nice at first, but it's
> > also confusing because the term "recover instruction" is used for the R*
> > instruction (and this is about the MIR instruction).
> > 
> > So, considering that, OnBailout(Path) seems less confusing...
> 
> I just noticed that part 4 uses isRecoverable/setRecoverable. Using
> OnBailout(Path) is not a good name then, unless we rename Recoverable too;
> they should have names that sound similar.

I think I will go for:
  isRecoveredOnBailout  (part 2)
  setRecoveredOnBailout
  canRecoverOnBailout   (part 4)

What do you think?
(In reply to Nicolas B. Pierron [:nbp] from comment #20)
> I think I will go for:
>   isRecoveredOnBailout  (part 2)
>   setRecoveredOnBailout
>   canRecoverOnBailout   (part 4)
> 
> What do you think?

Sounds good!
(Assignee)

Comment 22

3 years ago
Created attachment 8408953 [details] [diff] [review]
[part 4] DCE Additions captured by resume points.

Delta:
 - Moved the assertions into a macro of part 3.
 - Assert on the type of recovered values.
 - Rename isRecoverable to canRecoverOnBailout.
 - Check canRecoverOnBailout before writing.
 - Add supports for strings and floats.
Attachment #8400913 - Attachment is obsolete: true
Attachment #8400913 - Flags: feedback?(kvijayan)
Attachment #8400913 - Flags: feedback?(efaustbmo)
Attachment #8408953 - Flags: review?(jdemooij)
Attachment #8408953 - Flags: review?(hv1989)
Comment on attachment 8408953 [details] [diff] [review]
[part 4] DCE Additions captured by resume points.

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

Nice!

::: js/src/jit/MIR.h
@@ +3949,5 @@
>      bool isOperandTruncated(size_t index) const;
> +
> +    bool writeRecoverData(CompactBufferWriter &writer) const;
> +    bool canRecoverOnBailout() const {
> +        return specialization_ < MIRType_Object;

Awesome to see MIRType_Float also implemented!

::: js/src/jit/Recover.cpp
@@ +158,5 @@
> +
> +    // MIRType_Float32 is a specialization embeddeds the fact that the result is
> +    // rounded to a Float32.
> +    if (isFloatOperation_ && !RoundFloat32(cx, result, &result))
> +        return false;

This is the fix bbouvier agreed to, right. If not, please request f? or r? to confirm this line.
Attachment #8408953 - Flags: review?(hv1989) → review+
Comment on attachment 8408953 [details] [diff] [review]
[part 4] DCE Additions captured by resume points.

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

::: js/src/jit/Recover.cpp
@@ +155,5 @@
> +    MOZ_ASSERT(!lhs.isObject() && !rhs.isObject());
> +    if (!js::AddValues(cx, &lhs, &rhs, &result))
> +        return false;
> +
> +    // MIRType_Float32 is a specialization embeddeds the fact that the result is

Nit: embedding. Also, I assume tests fail without this? If not, please add such tests.

::: js/src/jsmath.cpp
@@ +589,5 @@
>      return success;
>  }
>  
>  bool
> +js::RoundFloat32(JSContext *cx, Handle<Value> arg, MutableHandle<Value> res)

Nit: HandleValue, MutableHandleValue. Please also fix the other RoundFloat32 overload while you're here.
Attachment #8408953 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 25

3 years ago
(In reply to Hannes Verschore [:h4writer] from comment #23)
> ::: js/src/jit/Recover.cpp
> @@ +158,5 @@
> > +
> > +    // MIRType_Float32 is a specialization embeddeds the fact that the result is
> > +    // rounded to a Float32.
> > +    if (isFloatOperation_ && !RoundFloat32(cx, result, &result))
> > +        return false;
> 
> This is the fix bbouvier agreed to, right. If not, please request f? or r?
> to confirm this line.

Yes, and it works fine with the modified test case, which is failing if I comment these lines.

(In reply to Jan de Mooij [:jandem] from comment #24)
> Also, I assume tests fail without this? If not, please add
> such tests.

I modified one of the test case to make sure the result is correct. Also, I added additional additions otherwise the condition needed to use Float32 optimizations implies that we will always get the same results.
(Assignee)

Comment 26

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fb280a72bb1
https://hg.mozilla.org/integration/mozilla-inbound/rev/85b6c3b4b26d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7977e7f8a094
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a3e7ed0c4c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b76df1986a6

https://tbpl.mozilla.org/?tree=Try&rev=d12820495fec
Keywords: feature
https://hg.mozilla.org/mozilla-central/rev/2fb280a72bb1
https://hg.mozilla.org/mozilla-central/rev/85b6c3b4b26d
https://hg.mozilla.org/mozilla-central/rev/7977e7f8a094
https://hg.mozilla.org/mozilla-central/rev/8a3e7ed0c4c1
https://hg.mozilla.org/mozilla-central/rev/6b76df1986a6
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(Assignee)

Updated

3 years ago
Depends on: 1003694
(Assignee)

Comment 28

3 years ago
Created attachment 8415150 [details] [review]
Iongraph: shade RecoveredOnBailout instructions.
Attachment #8415150 - Flags: review?(sstangl)
(Assignee)

Updated

3 years ago
Blocks: 1003801
(Assignee)

Updated

3 years ago
Depends on: 1006381
Comment on attachment 8415150 [details] [review]
Iongraph: shade RecoveredOnBailout instructions.

merged. Thanks!
Attachment #8415150 - Flags: review?(sstangl) → review+
(Assignee)

Updated

3 years ago
Depends on: 1026460
(Assignee)

Comment 30

3 years ago
feature
(feature keyword) =>
For release notes purposes:
https://blog.mozilla.org/javascript/2014/07/15/ionmonkey-optimizing-away/
You need to log in before you can comment on or make changes to this bug.