Closed Bug 1244254 Opened 4 years ago Closed 4 years ago

TSan: data race in SIMD CodeGen

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: terrence, Assigned: jolesen)

References

Details

Attachments

(9 files, 4 obsolete files)

6.49 KB, text/plain
Details
6.06 KB, text/plain
Details
4.21 KB, patch
nbp
: review+
Details | Diff | Splinter Review
4.73 KB, patch
nbp
: review+
Details | Diff | Splinter Review
6.94 KB, patch
nbp
: review+
Details | Diff | Splinter Review
34.26 KB, patch
nbp
: review+
Details | Diff | Splinter Review
5.53 KB, patch
nbp
: review+
Details | Diff | Splinter Review
18.94 KB, patch
nbp
: review+
Details | Diff | Splinter Review
47.07 KB, patch
nbp
: review+
bbouvier
: feedback+
Details | Diff | Splinter Review
Attached file tsan_simd.txt
Off-main-thread codegen should not be looking up properties, as those can mutate at any time on the main thread.
Flags: needinfo?(bbouvier)
Attached patch simdtype.patch (obsolete) — Splinter Review
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Flags: needinfo?(bbouvier)
Attachment #8714297 - Flags: review?(nicolas.b.pierron)
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
Attachment #8714297 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8714297 - Attachment is obsolete: true
Attachment #8714717 - Flags: review?(nicolas.b.pierron)
Attachment #8714717 - Flags: review?(nicolas.b.pierron) → review+
I'm also getting this TSAN report quite frequently. Will it be addressed by the patches here, or should I file a second bug?
Flags: needinfo?(bbouvier)
Yes we can, thank you for giving me these reports!
Flags: needinfo?(bbouvier)
Attachment #8714717 - Attachment description: simdtype.patch → 1. Get rid of the offthread type descr read when writing recover data
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?
Attachment #8715305 - Flags: review?(nicolas.b.pierron)
Discussed this with :nbp. I think we should add a SimdType member field to MSimdBox.
Depends on: 1245547
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #8714717 - Attachment is obsolete: true
Attachment #8715305 - Attachment is obsolete: true
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?
Attachment #8715395 - Flags: feedback+
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.
(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.
Assignee: bbouvier → jolesen
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.
Attachment #8716555 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8716556 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8716557 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8716558 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8716559 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8716560 - Flags: review?(nicolas.b.pierron)
Attachment #8715395 - Attachment is obsolete: true
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.
Attachment #8716561 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8716555 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8716556 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8716557 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8716558 - Flags: review?(nicolas.b.pierron) → review+
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?
Attachment #8716559 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8716560 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8716561 - Flags: review?(nicolas.b.pierron)
Attachment #8716561 - Flags: review+
Attachment #8716561 - Flags: feedback?(bbouvier)
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
Attachment #8716561 - Flags: feedback?(bbouvier) → feedback+
(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.
(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.
(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.
Depends on: 1248503
You need to log in before you can comment on or make changes to this bug.