1244254 - TSan: data race in SIMD CodeGen

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Terrence Cole [:terrence]]

Attachment: tsan_simd.txt

Off-main-thread codegen should not be looking up properties, as those can mutate at any time on the main thread.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: simdtype.patch

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8714297 [details] [diff] [review]
simdtype.patch

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

::: js/src/jit/MIR.h
@@ +3279,5 @@
>          setMovable();
>          setResultType(MIRType_Object);
>          if (constraints)
>              setResultTypeSet(MakeSingletonTypeSet(constraints, templateObject));
> +        simdType_ = templateObject->typeDescr().as<SimdTypeDescr>().type();

This code path is also racy, because it can be created after IonBuilder.

https://dxr.mozilla.org/mozilla-central/search?q=MSimdBox%3A%3ANew

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1. Get rid of the offthread type descr read when writing recover data

Oops, indeed. Good catch!

I was about to submit a more complicated patch that added the SimdType to MSimdBox ctor, and then I realized we actually have so many helpers for SIMD that there already is one to infer the SimdType from the input's MIRType. Let's just do that, it's much simpler and accurate.

Comment 4

[Terrence Cole [:terrence]]

Attachment: tsan_simd_addendumKind.txt

I'm also getting this TSAN report quite frequently. Will it be addressed by the patches here, or should I file a second bug?

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Yes we can, thank you for giving me these reports!

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Get rid of the type descr read when registering the SIMD template

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8715305 [details] [diff] [review]
2. Get rid of the type descr read when registering the SIMD template

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

::: js/src/jit/CodeGenerator.cpp
@@ +5048,5 @@
>      InlineTypedObject* templateObject = lir->mir()->templateObject();
>      gc::InitialHeap initialHeap = lir->mir()->initialHeap();
>      MIRType type = lir->mir()->input()->type();
> +
> +    registerSimdTemplate(lir->mir()->templateSimdType());

Unfortunately the MIRType is not refined enough to support this use case.
Should we carry the SimdType, as extracted from IonBuilder?

Comment 8

[Jakob Stoklund Olesen [:jolesen]]

Discussed this with :nbp. I think we should add a SimdType member field to MSimdBox.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: WIP

This is the overall solution, which requires many more changes. Not done yet, there's still an issue with it: MSimdUnbox needs a SimdType parameter now as well (to get the SimdType in EagerSimdUnbox), and we don't have them in MaybeSimdUnbox. Maybe we could have a better solution, entirely different.

Comment 10

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8715395 [details] [diff] [review]
WIP

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

Doing a quick review.  Having the SimdType as part of the MSimdUnbox sounds fine to me.

This patch removes MIRTypeToSimdType, I am not sure what is the best between removing it and keeping a declaration with a comment, but no definition to raise a link time error.

::: js/src/jit/MIR.h
@@ +74,5 @@
> +      case SimdType::Uint32x4:
> +      case SimdType::Int32x4:     *mirType = MIRType_Int32x4;   return true;
> +      case SimdType::Float32x4:   *mirType = MIRType_Float32x4; return true;
> +      case SimdType::Bool32x4:    *mirType = MIRType_Bool32x4;  return true;
> +      default:                    return false;

MOZ_CRASH?

Don't we just want to break everything if we are not able to get MSimdBox / MSimdUnbox working?

Comment 11

[Jakob Stoklund Olesen [:jolesen]]

The SIMD functions are completely monomorphic in their SIMD-typed arguments. If the argument doesn't match the expected type, they must throw a TypeError. We don't need to more advanced mechanism in TypePolicy.cpp for these arguments.

Instead we should:

- Create the MSimdUnbox nodes aggressively for all the arguments when inlining SIMD functions.
- Make the MSimd* instructions require SIMD-typed arguments. Assert in the constructor, even.
- Turn MaybeSimdUnbox() in TypePolicy.cpp into an assertion, or even drop the SIMD type policies completely.

This would mean that all MSimdBox and MSimdUnbox instructions are created in MCallOptimize where we know the SimdType, or in EagerSimdUnbox.cpp where we can get the SimdType from the current MSimdUnbox node.

Comment 12

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> Don't we just want to break everything if we are not able to get MSimdBox /
> MSimdUnbox working?

Most of the time, yes.

In IonBuilder::inlineConstructSimdObject(), we want to politely decline to inline a SIMD constructor call for one of the SIMD types not yet supported in Ion. No crashing there.

Comment 13

[Jakob Stoklund Olesen [:jolesen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c5076ef18b7

Comment 14

[Jakob Stoklund Olesen [:jolesen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=033d59370153

Comment 15

[Jakob Stoklund Olesen [:jolesen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=55bcdf021c2d

Comment 16

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Move SimdTypeToMIRType into the header.

Provide two versions of this function:

- MaybeSimdTypeToMIRType returns false for unsupported SimdTypes.
- SimdTypeToMIRType asserts that the type is supported.

The conditional one is used in IonBuilder::inlineConstructSimdObject() to
provide a 'SimdTypeNotOptimized' outcome for JIT coach.

Comment 17

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Pass a SimdType to inlineSimd().

We now have functions to create a (MIRType, SimdSign) from a SimdType, so use
those instead.

Calling SimdTypeToMIRType() also has the benefit of verifying that the list of
supported types in this function is consistent with the types in InlinableNative.

Following patches will push the SimdType to MIRType conversion further down the
call tree.

Comment 18

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Add IonBuilder::unboxSimd().

This helper function inserts an MUnboxSimd node which checks the arguments to
an inlined SIMD operation.

Use it for inlineSimdCheck() which now takes a SimdType argument. Also make
inlineSimdCheck produce a value that is a box/unbox pair instead of simply
returning its argument. This means that any SIMD type checking and unboxing
will happen at the time the check() function is executed, rather than when its
result is used. This is usually what you want.

Comment 19

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Check SIMD arguments in IonBuilder.

When inlining SIMD operations that take a SIMD type as an argument, explicitly
insert a type checking MSimdUnbox using the new unboxSimd() function. This
requires passing down the SimdType argument from inlineSimd().

- Remove SimdSign and other arguments that can be inferred from the SimdType.

- Add a new function inlineSimdShift() to handle the shift operations where the
  second argument is a scalar.

- Add a GetSimdLanes() function to SIMD.h which counts the number of lanes in a
  SIMD type.

Comment 20

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Replace MaybeSimdUnbox with assertions.

The SIMD type policies no longer needd to insert MSimdUnbox instructions before
SIMD operations. This is already handled before the SIMD operations are created.

Assert that the MIRTypes are as expected instead.

Comment 21

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Add SimdType to MSimdBox and MSimdUnbox.

The MIRType is not specific enough for MSimdUnbox to distinguish signed from
unsigned SIMD types, and when generating code for a MSimdBox, we can't look at
the templateObject to get the SimdType because it belongs to a different thread.

Pass a SimdType to CodeGenerator::registerSimdTemplate() instead of inspecting
the template object.

Delete MIRTypeToSimdType() which can't be accurate because MIRType doesn't
carry signedness information.

Add an optimization to unboxSimd(): With a SimdType on MSimdBox, we can
recognize the very common pattern where unboxSimd() gets called with an MSimdBox
value. In the types check out, just reuse the MSimdBox input and don't even
inert the check code.

Comment 22

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Simplify MSimd* constructors.

Now that all the MSimd* instructions require unboxed arguments of the right
type, there is no need to explicitly pass in the specialization MIRType when
creating these instructions.

- Remove the specialization MIRType argument from instruction constructors,
  ::New() and ::NewAsmJS() functions.

- Add tighter argument checking assertions in the constructors.

Comment 23

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8716555 [details] [diff] [review]
Move SimdTypeToMIRType into the header.

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

::: js/src/jit/MIR.h
@@ +99,5 @@
> +//
> +// Note that this is not an injective mapping: SimdType has signed and unsigned
> +// integer types that map to the same MIRType.
> +static inline
> +MIRType SimdTypeToMIRType(SimdType type)

I think it would make sense to move these functions into js/jit/IonTypes.h.
Do not forget to add the corresponding header, which defines the SimdType enum.

Comment 24

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8716557 [details] [diff] [review]
Add IonBuilder::unboxSimd().

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

::: js/src/jit/MCallOptimize.cpp
@@ +3355,5 @@
>          return InliningStatus_NotInlined;
>  
> +    // Unboxing checks the SIMD object type and throws a TypeError if it doesn't
> +    // match type.
> +    MDefinition *arg = unboxSimd(callInfo.getArg(0), type);

nit: see below.

@@ +3377,5 @@
> +//
> +// This represents the standard type checking that all the SIMD operations
> +// perform on their arguments.
> +MDefinition*
> +IonBuilder::unboxSimd(MDefinition* ins, SimdType type)

nit: If you were to return an MInstruction* here, you would not have to …

@@ +3394,5 @@
>                                    templateObj->group()->initialHeap(constraints()));
>  
>      // In some cases, ins has already been added to current.
> +    if (!ins->block() && ins->isInstruction())
> +        current->add(ins->toInstruction());

… to change the MInstruction argument by an MDefinition.

Comment 25

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8716558 [details] [diff] [review]
Check SIMD arguments in IonBuilder.

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

::: js/src/jit/MCallOptimize.cpp
@@ +3154,3 @@
>        case SimdOperation::Fn_shiftRightByScalar:
> +          return inlineSimdShift(callInfo, native, MSimdShift::rshForSign(GetSimdSign(type)),
> +                                 type);

style-nit: indent by 2 and not 4.

Comment 26

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8716559 [details] [diff] [review]
Replace MaybeSimdUnbox with assertions.

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

::: js/src/jit/TypePolicy.cpp
@@ +823,1 @@
>      }

style-nit: remove the braces around single statements.

@@ +926,5 @@
>  {
> +    // Storing a SIMD value requires a valueOperand that has already been
> +    // SimdUnboxed. See IonBuilder::inlineSimdStore(()
> +    if (Scalar::isSimdType(writeType)) {
> +        MOZ_ASSERT(IsSimdType(value->type()));

nit: Should we check that writeType and value->type() are equal?

Comment 27

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8716560 [details] [diff] [review]
Add SimdType to MSimdBox and MSimdUnbox.

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

::: js/src/jit/EagerSimdUnbox.cpp
@@ +74,1 @@
>      InlineTypedObject* inlineTypedObject = &templateObject->as<InlineTypedObject>();

terrence: Could this line be racy knowing that this is being executed in the compiler?
I do not think we are reading anything from this object beyond casting it, and saving it as a templateObject for the compilation.

::: js/src/jit/MCallOptimize.cpp
@@ +3378,5 @@
> +        MSimdBox* box = ins->toSimdBox();
> +        if (box->simdType() == type) {
> +            MDefinition* value = box->input();
> +            MOZ_ASSERT(value->type() == SimdTypeToMIRType(type));
> +            return value;

ok, forget what I mentioned in the previous patch review, about changing the return type to a MInstruction*.

::: js/src/jit/MIR.cpp
@@ +998,5 @@
>          // If the operand is a MSimdBox, then we just reuse the operand of the
>          // MSimdBox as long as the type corresponds to what we are supposed to
>          // unbox.
> +        in = box->input();
> +        if (box->simdType() != simdType() || in->type() != type())

nit: simdType being a super set of the type, I suggest to only assert that the type() are equal.

::: js/src/jit/MIR.h
@@ +3394,5 @@
>      bool congruentTo(const MDefinition* ins) const override {
> +        if (!congruentIfOperandsEqual(ins))
> +            return false;
> +        const MSimdBox* box = ins->toSimdBox();
> +        return box->initialHeap() == initialHeap() && box->simdType() == simdType();

nit: The simdType discrimates more, and should be enough here.

Comment 28

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8716561 [details] [diff] [review]
Simplify MSimd* constructors.

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

This sounds good to me and it seems that we can now remove the MSimd*::NewAsmJS functions.
Benjamin, what do you think?

::: js/src/jit/MIR.h
@@ +2432,2 @@
>      {
>          MOZ_ASSERT(left->type() == MIRType_Int32x4 && right->type() == MIRType_Int32);

I guess we should remove this assertion as soon as we add support for Uint32x4 in AsmJS.

@@ +2494,2 @@
>      {
>          MOZ_ASSERT(mask->type() == MIRType_Bool32x4);

This assertion is redundant, now that we assert IsBooleanSimdType in the constructor.

Comment 29

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8716561 [details] [diff] [review]
Simplify MSimd* constructors.

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

Nice. Agreed that when asm.js support is implemented, we should be able to get rid of most NewAsmJS constructors and use the the New constructors instead.

::: js/src/jit/MIR.h
@@ +1666,5 @@
>    protected:
>      SimdLane lane_;
>      SimdSign sign_;
>  
> +    MSimdExtractElement(MDefinition* obj, MIRType laneType, SimdLane lane, SimdSign sign)

Could the laneType be deduced from the vector's type? (I assume so, even for unsigned types, as we don't have MIRType_Unsigned32 in our JIT)

@@ +1754,5 @@
>    public:
>      INSTRUCTION_HEADER(SimdInsertElement)
>  
>      static MSimdInsertElement* NewAsmJS(TempAllocator& alloc, MDefinition* vec, MDefinition* val,
> +                                         SimdLane lane)

nit: (pre-existing) please align SimdLane with TempAllocator above

Comment 30

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #29)
> Comment on attachment 8716561 [details] [diff] [review]

> > +    MSimdExtractElement(MDefinition* obj, MIRType laneType, SimdLane lane, SimdSign sign)
> 
> Could the laneType be deduced from the vector's type? (I assume so, even for
> unsigned types, as we don't have MIRType_Unsigned32 in our JIT)

There are two cases where we can't do that:

- Uint32x4.extractLane() produces a MIRType_Double for JS and MIRType_Int32 for asm.

- Bool32x4.extractLane() produces a MIRType_Boolean for JS and MIRType_Int32 for asm.

If we're going to remove the NewAsmJS factories, I think we should keep the laneType argument instead.

Comment 31

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Nicolas B. Pierron [:nbp] from comment #23)
> Comment on attachment 8716555 [details] [diff] [review]
> Move SimdTypeToMIRType into the header.
> 
> Review of attachment 8716555 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/MIR.h
> @@ +99,5 @@
> > +//
> > +// Note that this is not an injective mapping: SimdType has signed and unsigned
> > +// integer types that map to the same MIRType.
> > +static inline
> > +MIRType SimdTypeToMIRType(SimdType type)
> 
> I think it would make sense to move these functions into js/jit/IonTypes.h.
> Do not forget to add the corresponding header, which defines the SimdType
> enum.

I agree that would makes sense, but apparently including "builtin/SIMD.h" from IonTypes.h causes a dependency loop with vm/TypeInference.h which needs MIRType.

Comment 32

[Jakob Stoklund Olesen [:jolesen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=615cfffc3b76

Comment 33

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Nicolas B. Pierron [:nbp] from comment #27)
> Comment on attachment 8716560 [details] [diff] [review]
> Add SimdType to MSimdBox and MSimdUnbox.
> 
> Review of attachment 8716560 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/EagerSimdUnbox.cpp
> @@ +74,1 @@
> >      InlineTypedObject* inlineTypedObject = &templateObject->as<InlineTypedObject>();
> 
> terrence: Could this line be racy knowing that this is being executed in the
> compiler?
> I do not think we are reading anything from this object beyond casting it,
> and saving it as a templateObject for the compilation.

If this is unsafe, I suppose it would be possible to attach a template object pointer to MSimdUnbox too. Then EagerSimdUnbox would always have access to a template object.

Comment 34

[Jakob Stoklund Olesen [:jolesen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7eabb9a77994b362daffe3eb588770960a99e4f3
Bug 1244254 - Move SimdTypeToMIRType into the header. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/3104d4155b1e9f2ec92286fcfa380dc5ba540f5f
Bug 1244254 - Pass a SimdType to inlineSimd(). r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbf37b8608533bc4a652e0055618dd2b2080aeb2
Bug 1244254 - Add IonBuilder::unboxSimd(). r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/65c42ca17bc765a219d793c7e27be46bc8400a3e
Bug 1244254 - Check SIMD arguments in IonBuilder. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/d79a8c190b860f0cda2f3f2e71d7619936d45a79
Bug 1244254 - Replace MaybeSimdUnbox with assertions. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e9789224055886c7a1a7126ca4e3428ce00d42e
Bug 1244254 - Add SimdType to MSimdBox and MSimdUnbox. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ecabf42762c4a152dd32cd53280d27e84faef2
Bug 1244254 - Simplify MSimd* constructors. r=nbp

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7eabb9a77994
https://hg.mozilla.org/mozilla-central/rev/3104d4155b1e
https://hg.mozilla.org/mozilla-central/rev/dbf37b860853
https://hg.mozilla.org/mozilla-central/rev/65c42ca17bc7
https://hg.mozilla.org/mozilla-central/rev/d79a8c190b86
https://hg.mozilla.org/mozilla-central/rev/6e9789224055
https://hg.mozilla.org/mozilla-central/rev/b7ecabf42762