Bug 228675 - Add new patterns to instruction selector to utilize AND/EOR/ORR-with-shift supported by ARM64
Summary: Add new patterns to instruction selector to utilize AND/EOR/ORR-with-shift su...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yijia Huang
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-30 20:59 PDT by Yijia Huang
Modified: 2021-08-02 22:42 PDT (History)
7 users (show)

See Also:


Attachments
Patch (80.62 KB, patch)
2021-07-30 21:01 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (83.88 KB, patch)
2021-07-31 07:53 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (87.49 KB, patch)
2021-07-31 20:54 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (91.04 KB, patch)
2021-08-02 15:52 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (64.87 KB, patch)
2021-08-02 17:15 PDT, Yijia Huang
sbarati: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (64.73 KB, patch)
2021-08-02 19:17 PDT, Yijia Huang
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (64.72 KB, patch)
2021-08-02 19:21 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yijia Huang 2021-07-30 20:59:22 PDT
...
Comment 1 Yijia Huang 2021-07-30 21:01:57 PDT
Created attachment 434688 [details]
Patch
Comment 2 Mark Lam 2021-07-30 21:24:44 PDT
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?
Comment 3 Yijia Huang 2021-07-31 07:53:08 PDT
Created attachment 434698 [details]
Patch
Comment 4 Yijia Huang 2021-07-31 20:54:44 PDT
Created attachment 434713 [details]
Patch
Comment 5 Saam Barati 2021-08-02 14:49:00 PDT
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.
Comment 6 Yijia Huang 2021-08-02 15:52:59 PDT
Created attachment 434795 [details]
Patch
Comment 7 Yijia Huang 2021-08-02 17:15:27 PDT
Created attachment 434806 [details]
Patch
Comment 8 Saam Barati 2021-08-02 18:55:22 PDT
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.
Comment 9 Yijia Huang 2021-08-02 19:17:56 PDT
Created attachment 434809 [details]
Patch for landing
Comment 10 Yijia Huang 2021-08-02 19:21:24 PDT
Created attachment 434810 [details]
Patch for landing
Comment 11 EWS 2021-08-02 20:11:45 PDT
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].
Comment 12 Radar WebKit Bug Importer 2021-08-02 20:17:20 PDT
<rdar://problem/81441719>