Bug 228538 – [ARM64] Add Pre/Post-Indexed Address Mode to Air for ARM64 (Store Instruction)

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

[Yijia Huang]

...

[Yijia Huang]

Created attachment 434407 [details]
Patch

[Yijia Huang]

Created attachment 434731 [details]
Patch

[Yijia Huang]

Created attachment 434769 [details]
Patch

[Yijia Huang]

Created attachment 434792 [details]
Patch

[Yijia Huang]

Created attachment 434818 [details]
Patch

[Yijia Huang]

Created attachment 434820 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/81500926>

[Yijia Huang]

Created attachment 435056 [details]
Patch

[Saam Barati]

Comment on attachment 434820 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        , which benefits loop program. Here, this patch adds the corresponding mode for Store

this comma should be on the previous line

[Yijia Huang]

Created attachment 435124 [details]
Patch

[Yijia Huang]

Created attachment 435238 [details]
Patch

[Yijia Huang]

Created attachment 435261 [details]
Patch

[EWS]

Committed r281062 (240524@main): <https://commits.webkit.org/240524@main>

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

[Saam Barati]

Comment on attachment 435261 [details]
Patch

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

> Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:103
> +            //     memory = Store(value, base, offset)

I don't understand this comment since we're skipping things with offsets below.

> Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:112
> +                if (memory->child(0)->hasInt() || memory->offset())

Why do we care about "memory->child(0)->hasInt()" here? Doesn't that just mean that we're storing a constant?

> Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:145
> +            tryAddpostIndexCandidates();

I think the "p" should be capitalized here

> Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:165
> +    // This search is expensive. However, due to the greedy pattern
> +    // matching, no better method can be proposed at present.

I don't get this comment. We know what items we look up here. We also know their index when we are traversing the basic block. We could easily keep a table of these records (e.g, when we see an add, keep a table of add/memory->index in block)

> Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:173
>          return index;

If we don't do above, this should be a RELEASE_ASSERT_NOT_REACHED, and the index value can be local to the block it's in.

> Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:184
> +        // address = Add(base, offset)       address = Add(base, offset)
> +        // ...                          -->  newMemory = Load(base, offset)
> +        // ...                               ...
> +        // memory = Load(base, offset)       memory = Identity(newMemory)

This looks super wrong. What if the "..." contains a store to a memory location that aliases? Moving the load above the store is semantically incorrect because it's not identical to the behavior of the original program.