Bug 171767 – Air::ShufflePair::inst is wrong

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: JavaScriptCore

[Saam Barati]

It may return an invalid Inst.

[Saam Barati]

Created attachment 309259 [details]
WIP

Still need to test on ARM where things are actually interesting.

[Filip Pizlo]

Comment on attachment 309259 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=309259&action=review

I like how this turned out.

> Source/JavaScriptCore/b3/air/AirEmitShuffle.cpp:106
> -Inst ShufflePair::inst(Code* code, Value* origin) const
> +Vector<Inst> ShufflePair::insts(Code& code, Value* origin) const
>  {
> -    if (UNLIKELY(src().isMemory() && dst().isMemory())) {
> -        RELEASE_ASSERT(code);
> -        return Inst(moveFor(bank(), width()), origin, src(), dst(), code->newTmp(bank()));
> -    }
> +    // FIXME: This function should really worry about encodable offsets.
> +
> +    if (UNLIKELY(src().isMemory() && dst().isMemory()))
> +        return { Inst(moveFor(bank(), width()), origin, src(), dst(), code.newTmp(bank())) };
>  
> -    if (src().isSomeImm())
> -        return Inst(Move, origin, Arg::bigImm(src().value()), dst());
> +    if (isValidForm(moveFor(bank(), width()), src().kind(), dst().kind()))
> +        return { Inst(moveFor(bank(), width()), origin, src(), dst()) };
>  
> -    return Inst(moveFor(bank(), width()), origin, src(), dst());
> +    // We must be a store immediate or a move immediate to tmp if we reach here. The reason:
> +    // 1. It's always valid to do a load from Addr into a tmp using Move/Move32/MoveFloat/MoveDouble.
> +    // 2. It's also always valid to do a Tmp->Tmp move.
> +    // 3. It's also always valid to do a Tmp->Addr store.
> +    ASSERT(src().isSomeImm());
> +    Tmp tmp = code.newTmp(bank());
> +    ASSERT(isValidForm(Move, Arg::BigImm, Arg::Tmp));
> +    ASSERT(isValidForm(moveFor(bank(), width()), Arg::Tmp, dst().kind()));
> +    return {
> +        Inst(Move, origin, Arg::bigImm(src().value()), tmp),
> +        Inst(moveFor(bank(), width()), origin, tmp, dst()),
> +    };
>  }

I was super curious how the full shuffler handles this, and it looks like it has exactly this logic in handleShiftPair.  Up to you if you want to try to unify them.  It could get ugly - you want to use a tmp, that code wants to find a scratch - so it's OK if they are separate.

[Filip Pizlo]

Comment on attachment 309259 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=309259&action=review

> Source/JavaScriptCore/b3/air/AirEmitShuffle.h:79
> +    Vector<Inst> insts(Code&, Value* origin) const;

How about Vector<Inst, 2>

Since it's never than 2 insts.

[Saam Barati]

*** This bug has been marked as a duplicate of bug 171392 ***