Bug 227970 – Add ExtendType to Air::Arg Index to fully utilize address computation in memory instruction for ARM64

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

[Yijia Huang]

...

[Yijia Huang]

Created attachment 433545 [details]
Patch

[Yijia Huang]

Created attachment 433566 [details]
Patch

[Yijia Huang]

Created attachment 433603 [details]
Patch

[Yijia Huang]

Created attachment 433614 [details]
Patch

[Saam Barati]

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?

[Saam Barati]

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?

[Saam Barati]

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

[Yijia Huang]

Created attachment 433640 [details]
Patch

[Yijia Huang]

Created attachment 433642 [details]
Patch

[Yijia Huang]

Created attachment 433651 [details]
Patch

[Yijia Huang]

Created attachment 433655 [details]
Patch

[Yijia Huang]

Created attachment 433668 [details]
Patch

[Yijia Huang]

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.

[Yijia Huang]

Created attachment 433688 [details]
Patch

[Saam Barati]

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.

[Yijia Huang]

Created attachment 433706 [details]
Patch

[Saam Barati]

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?

[Yijia Huang]

Created attachment 433715 [details]
Patch for landing

[Yijia Huang]

Created attachment 433716 [details]
Patch for landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/80711279>