1112153 - ResumePoint / Snapshot: Forbid MIRType_Int32x4 / MIRType_Float32x4.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

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.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Verify that all resume points / recover instructions operands can be encoded in snapshots.

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.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

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()), ...);

Comment 3

[Nicolas B. Pierron [:nbp]]

(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.

Comment 4

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55eab7996db9

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55eab7996db9