Bug 228675 – Add new patterns to instruction selector to utilize AND/EOR/ORR-with-shift supported by ARM64

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yijia Huang]

...

[Yijia Huang]

Created attachment 434688 [details]
Patch

[Mark Lam]

Comment on attachment 434688 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        Add new patternes to instruction selector to utilize AND/EOR/ORR-with-shift and UMULL supported by ARM64

nit: typo: /patternes/patterns/.  Can you fix the bugzilla title as well?

[Yijia Huang]

Created attachment 434698 [details]
Patch

[Yijia Huang]

Created attachment 434713 [details]
Patch

[Saam Barati]

Comment on attachment 434713 [details]
Patch

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

New bitop+shift selection looks good. One suggestion there.

But I'm confused by changes in call sites of isMergeableValue

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2675
> +                        if (isMergeableValue(multiplyLeft, SExt32) && isMergeableValue(multiplyRight, SExt32)) 

How is this behavior change sound? We're no longer checking canBeInternal, but we're still calling commitInternal.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2678
> +                        if (isMergeableValue(multiplyLeft, ZExt32) && isMergeableValue(multiplyRight, ZExt32)) 

ditto

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2753
> +                    if (isMergeableValue(multiplyLeft, SExt32) && isMergeableValue(multiplyRight, SExt32)) 

ditto

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2876
> +                    commitInternal(left);
> +                    commitInternal(right);

ditto regarding not checking canBeInternal.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2998
> +            // and-with-shift Pattern: n & (m ShiftType amount)

nit: I'd use the names the function uses in this comment:
"and-with-shift Pattern: left & (right ShiftType amount)"

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:3284
> +            auto tryAppendXorWithShift = [&] (Value* left, Value* right) -> bool {

tryAppendXorWithShift and tryAppendAndWithShift and tryAppendOrWithShift are all identical minus the opcodes passed to the opcodeBasedOnShiftKind call. Can we abstract these 3 into a function shared by all? Maybe tryAppendBitOpWithShift or something?

You can still keep all 3 lambdas, just so it's easier to call with (left, right) and (right, left), but the lambdas can call a shared helper.

[Yijia Huang]

Created attachment 434795 [details]
Patch

[Yijia Huang]

Created attachment 434806 [details]
Patch

[Saam Barati]

Comment on attachment 434806 [details]
Patch

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

Nice. r=me

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2550
> +        auto tryAppendBitOpWithShift = [&] (Value* left, Value* right, Air::Opcode opcode) -> bool {
> +            if (!isValidForm(opcode, Arg::Tmp, Arg::Tmp, Arg::Imm, Arg::Tmp))
> +                return false;
> +            if (!canBeInternal(right) || !imm(right->child(1)) || right->child(1)->asInt() < 0)
> +                return false;
> +
> +            uint64_t amount = right->child(1)->asInt();
> +            uint64_t datasize = m_value->type() == Int32 ? 32 : 64;
> +            if (amount >= datasize)
> +                return false;
> +
> +            append(opcode, tmp(left), tmp(right->child(0)), imm(right->child(1)), tmp(m_value));
> +            commitInternal(right);
> +            return true;
> +        };

I think the way we normally do this is just by making it a member function.

[Yijia Huang]

Created attachment 434809 [details]
Patch for landing

[Yijia Huang]

Created attachment 434810 [details]
Patch for landing

[EWS]

Committed r280579 (240201@main): <https://commits.webkit.org/240201@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434810 [details].

[Radar WebKit Bug Importer]

<rdar://problem/81441719>