Bug 227970 - Add ExtendType to Air::Arg Index to fully utilize address computation in memory instruction for ARM64
Summary: Add ExtendType to Air::Arg Index to fully utilize address computation in memo...
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-14 15:53 PDT by Yijia Huang
Modified: 2021-07-16 17:54 PDT (History)
8 users (show)

See Also:


Attachments
Patch (6.04 KB, patch)
2021-07-14 17:06 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (14.25 KB, patch)
2021-07-15 00:08 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (14.69 KB, patch)
2021-07-15 11:37 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (17.52 KB, patch)
2021-07-15 12:59 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (21.82 KB, patch)
2021-07-15 17:17 PDT, Yijia Huang
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (21.86 KB, patch)
2021-07-15 17:36 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (21.86 KB, patch)
2021-07-15 20:40 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (27.60 KB, patch)
2021-07-15 22:04 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (32.56 KB, patch)
2021-07-16 02:52 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (40.78 KB, patch)
2021-07-16 11:32 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (40.91 KB, patch)
2021-07-16 14:46 PDT, Yijia Huang
sbarati: review+
Details | Formatted Diff | Diff
Patch for landing (41.12 KB, patch)
2021-07-16 16:37 PDT, Yijia Huang
yijia_huang: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (41.13 KB, patch)
2021-07-16 17:08 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-14 15:53:52 PDT
...
Comment 1 Yijia Huang 2021-07-14 17:06:27 PDT
Created attachment 433545 [details]
Patch
Comment 2 Yijia Huang 2021-07-15 00:08:39 PDT
Created attachment 433566 [details]
Patch
Comment 3 Yijia Huang 2021-07-15 11:37:54 PDT
Created attachment 433603 [details]
Patch
Comment 4 Yijia Huang 2021-07-15 12:59:02 PDT
Created attachment 433614 [details]
Patch
Comment 5 Saam Barati 2021-07-15 15:56:51 PDT
Comment on attachment 433614 [details]
Patch

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

> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:130
> +        SExt,

Is this a thing?

> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:131
> +        Oops

Can this be called None?

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:556
> +#if CPU(ARM64)
> +                if (index->opcode() == ZExt32 && !m_locked.contains(index->child(0)))
> +                    return Arg::index(tmp(base), tmp(index->child(0)), *scale, offset, MacroAssembler::ZExt);
> +#endif

I feel like the more idiomatic Air way to do this is to hook into isValidIndexForm and teach it about ZExt on arm64.

> Source/JavaScriptCore/b3/air/AirArg.h:1491
>      int32_t m_scale { 1 };
>      Air::Tmp m_base;
>      Air::Tmp m_index;
> +    MacroAssembler::Extend m_extend = MacroAssembler::Extend::Oops;

I think we only need a single bit for "m_extend" right now. I wonder if we can steal that from elsewhere. One bit for SExt vs nothing. Do we use all 32-bits of m_scale?
Comment 6 Saam Barati 2021-07-15 16:20:58 PDT
Comment on attachment 433614 [details]
Patch

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

>> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:130
>> +        SExt,
> 
> Is this a thing?

I think architecturally it is. But do we ever use it?
Comment 7 Saam Barati 2021-07-15 16:21:57 PDT
Comment on attachment 433614 [details]
Patch

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

>>> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:130
>>> +        SExt,
>> 
>> Is this a thing?
> 
> I think architecturally it is. But do we ever use it?

maybe it's useful when we do loads of negative properties? I guess we can just add support
Comment 8 Yijia Huang 2021-07-15 17:17:49 PDT
Created attachment 433640 [details]
Patch
Comment 9 Yijia Huang 2021-07-15 17:36:29 PDT
Created attachment 433642 [details]
Patch
Comment 10 Yijia Huang 2021-07-15 20:40:49 PDT
Created attachment 433651 [details]
Patch
Comment 11 Yijia Huang 2021-07-15 22:04:42 PDT
Created attachment 433655 [details]
Patch
Comment 12 Yijia Huang 2021-07-16 02:52:09 PDT
Created attachment 433668 [details]
Patch
Comment 13 Yijia Huang 2021-07-16 10:27:37 PDT
Comment on attachment 433614 [details]
Patch

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

>> Source/JavaScriptCore/b3/B3LowerToAir.cpp:556
>> +#endif
> 
> I feel like the more idiomatic Air way to do this is to hook into isValidIndexForm and teach it about ZExt on arm64.

I agree that it should be hooked to isValidIndexForm. But maybe introducing it to Air opcode is overkill for the extended index at the moment. Please correct me if I am wrong.
Comment 14 Yijia Huang 2021-07-16 11:32:11 PDT
Created attachment 433688 [details]
Patch
Comment 15 Saam Barati 2021-07-16 14:24:19 PDT
Comment on attachment 433688 [details]
Patch

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

> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:128
> +    enum Extend {

enum class Extend : uint8_t

> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:226
> +            , extend(extend)
>          {
>          }

if we're on x86, can we assert that it's None?

Maybe in the future, we can change x86's assembler to handle not None type, by having it emit code using the temp register. But for now, we should assert it's not being used.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1377
> +        default:

nit: I like to list all cases here, then omit the default case. That way, the compiler will warn if you're missing a case.

I think I'd write this then as:
"case None"

> Source/JavaScriptCore/b3/air/AirArg.h:1491
> +    MacroAssembler::Extend m_extend = MacroAssembler::Extend::None;

This field should go after m_kind for for better packing size.

also "m_extend { MacroAssembler::Extend::None };" is how we write this.
Comment 16 Yijia Huang 2021-07-16 14:46:40 PDT
Created attachment 433706 [details]
Patch
Comment 17 Saam Barati 2021-07-16 15:30:36 PDT
Comment on attachment 433706 [details]
Patch

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

r=me with comments

> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:128
> +    enum Extend : uint8_t {

enum class

> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:228
> +#if CPU(X86) || CPU(X86_64)
> +            ASSERT(extend == None);
> +#endif

Sorry, I know I said differently earlier. But since you only support this on arm64, I think I'd actually do this as:

#if !CPU(ARM64)

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:577
> +                return indexArg(tmp(base), index->child(0), *scale, offset);

can we add a test for add here?
Comment 18 Yijia Huang 2021-07-16 16:37:34 PDT
Created attachment 433715 [details]
Patch for landing
Comment 19 Yijia Huang 2021-07-16 17:08:11 PDT
Created attachment 433716 [details]
Patch for landing
Comment 20 EWS 2021-07-16 17:53:11 PDT
Committed r280013 (239755@main): <https://commits.webkit.org/239755@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433716 [details].
Comment 21 Radar WebKit Bug Importer 2021-07-16 17:54:17 PDT
<rdar://problem/80711279>