Closed Bug 1112153 Opened 7 years ago Closed 7 years ago

ResumePoint / Snapshot: Forbid MIRType_Int32x4 / MIRType_Float32x4.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file)

Snapshots are used to represent values, at the moment, the goal is to inline
the SIMD operations, without doing any optimization which might cause any
stack alignment issue.  To make sure we are not eagerly saving SIMD registers,
we should manipulate these registers such that they cannot be spilled on the
stack.
This patch mirror CodeGeneratorShared::encodeAllocation for all operands
which are encoded in a snapshot.  It assert after every phase that resume
point / recover instruction operands are consistent with what can be encoded
in a RValueAllocation.

For now, this would be useful to ensure that we do not use a MIRType_Int32x4
instruction as operand of a resume point, as we are unable to encode it.
Attachment #8537943 - Flags: review?(benj)
Comment on attachment 8537943 [details] [diff] [review]
Verify that all resume points / recover instructions operands can be encoded in snapshots.

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

Thanks. It feels sane to have assertions to avoid these cases now.

::: js/src/jit/IonAnalysis.cpp
@@ +2056,5 @@
> +      case MIRType_Value:
> +        return true;
> +      default:
> +        return false;
> +    }

See comment below, but if you prefer to keep this function with the switch,
1) Can you capitalize the first letter of the function's name?
2) Can you make all cases explicit (i.e. no more default), such that when we add a new value in the MIRType enum, we get a warning / compile error?

@@ +2066,5 @@
> +    for (size_t i = 0, e = node->numOperands(); i < e; ++i) {
> +        MDefinition *op = node->getOperand(i);
> +        if (op->isRecoveredOnBailout())
> +            continue;
> +        MOZ_ASSERT(isResumableMIRType(op->type()),

With IsSimdType(MIRType type), you can even get rid of isResumableMIRType:
  MOZ_ASSERT(!IsSimdType(op->type()), ...);
Attachment #8537943 - Flags: review?(benj) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> See comment below, but if you prefer to keep this function with the switch,
> 1) Can you capitalize the first letter of the function's name?
> 2) Can you make all cases explicit (i.e. no more default), such that when we
> add a new value in the MIRType enum, we get a warning / compile error?

Done.

> With IsSimdType(MIRType type), you can even get rid of isResumableMIRType:
>   MOZ_ASSERT(!IsSimdType(op->type()), ...);

I prefer the IsResumableMIRType function as it maintains the invariant expected by CodeGeneratorShared::encodeAllocation, and not only a subset of this invariant.
https://hg.mozilla.org/mozilla-central/rev/55eab7996db9
Assignee: nobody → nicolas.b.pierron
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.