Bug 227204 – Add a new pattern to instruction selector to utilize UBFIZ supported by ARM64

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

[Yijia Huang]

...

[Yijia Huang]

Created attachment 431934 [details]
Patch

[Saam Barati]

Comment on attachment 431934 [details]
Patch

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

Nice. r=me

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2761
> +                uint64_t width = static_cast<uint64_t>(WTF::bitCount(mask));

nit: this static cast isn't needed

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2795
> +                uint64_t width = static_cast<uint64_t>(WTF::bitCount(mask));

nit: this static_cast isn't needed

[Saam Barati]

Comment on attachment 431934 [details]
Patch

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

> Source/WTF/wtf/MathExtras.h:741
> +inline size_t countTrailingZeros(uint32_t v)
> +{
> +    static const unsigned Mod37BitPosition[] = {
> +        32, 0, 1, 26, 2, 23, 27, 0, 3, 16, 24, 30, 28, 11, 0, 13,
> +        4, 7, 17, 0, 25, 22, 31, 15, 29, 10, 12, 6, 0, 21, 14, 9,
> +        5, 20, 8, 19, 18
> +    };
> +    return Mod37BitPosition[((1 + ~v) & v) % 37];
> +}
> +
> +inline size_t countTrailingZeros(uint64_t v)

should we use builtin_ctz when compiling with GCC or clang so the proper instruction can be selected?

[Yijia Huang]

Created attachment 431939 [details]
Patch

[Filip Pizlo]

Comment on attachment 431939 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:19
> +        d = (n << lsb) & mask

Why not instead select (n & mask) << lsb?

Then, you can canonicalize (n << lsb) & mask to (n & mask) << lsb in reduceStrength.

The benefit of (n & mask) << lsb being the canonical form is it's easier to recognize. So, if some other part of the compiler needed to recognize this pattern, they'd only have to recognize the easier one of them.

[Yijia Huang]

Created attachment 432014 [details]
Patch

[Saam Barati]

Comment on attachment 432014 [details]
Patch

Let's figure out the crash

[Yijia Huang]

Created attachment 432059 [details]
Patch

[Filip Pizlo]

Comment on attachment 432059 [details]
Patch

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

This looks good but you're matching rules for BitAnd that won't happen after reduceStrength does handleCommutativity.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2753
> +                if (imm(srcValue) || !imm(lsbValue) || lsbValue->asInt() < 0 || !right->hasInt())

You don't need to exit if srcValue has an imm.  If it had an imm, then it wouldn't have had ZShr as its opcode.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2770
> +            if (tryAppendUBFX(left, right) || tryAppendUBFX(right, left))

You can be sure that BitAnd will have its immediate on the right.

I believe we have a reduceStrength rule for that:

        case BitAnd:
            handleCommutativity();

handleCommutativity() will flip the BitAnd to put the immediate on the right if it wasn't on the right already.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2829
> +                    if (imm(nValue) || !maskValue->hasInt() || !imm(right) || right->asInt() < 0)

No need to bail out of imm() returns true.  If both sides are imm then the whole thing would have been constant folded and you wouldn't have even seen a BitAnd.  And if it wasn't constant folded for any reason, then there's no downside to you matching this rule.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2846
> +                return tryAndValueMask(andLeft, andRight) || tryAndValueMask(andRight, andLeft);

Ditto - only have to match the case where the immediate is on the right.

[Yijia Huang]

Created attachment 432089 [details]
Patch

[Yijia Huang]

Thanks for the review!

[EWS]

Committed r279193 (239084@main): <https://commits.webkit.org/239084@main>

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