Bug 228890 – [ARM64] Clean up and fix Pre/Post-Indexed Address Mode to Air for ARM64 (Load Instruction)

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

[Yijia Huang]

...

[Yijia Huang]

Created attachment 435116 [details]
Patch

[Mark Lam]

Comment on attachment 435116 [details]
Patch

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

> Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:142
> +    auto detect = [&] (const HashMap<MemoryValue*, Value*>& candidates, bool isPreIndexCandidiates) {

Typo: /isPreIndexCandidiates/isPreIndexCandidates/

> Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:165
> +                if (!controlEquivalent(memory, address->child(1)))

Can you put a brief description in the ChangeLog as to why this is needed?

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2585
> +            //     memory = load(base, 0)                MoveWithIncrement (%address, posfix(offset)) %memory

/posfix/postfix/

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2591
> +                if (m_locked.contains(address) || address->opcode() != Add || address->type() != Int64)

Please put a brief description in the ChangeLog as to why this m_locked check is needed.

> Source/JavaScriptCore/b3/air/AirArg.h:1319
> +            return isValidIncrementIndexForm(offset());

You can remove this line too.

[Yijia Huang]

Created attachment 435137 [details]
Patch

[Yijia Huang]

(In reply to Mark Lam from comment #2)
> Comment on attachment 435116 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435116&action=review
> 

Thanks for reviewing. Updated.

[Yijia Huang]

Created attachment 435139 [details]
Patch

[Yijia Huang]

Created attachment 435142 [details]
Patch

[Yijia Huang]

Created attachment 435143 [details]
Patch

[Yijia Huang]

Created attachment 435148 [details]
Patch

[Yijia Huang]

Created attachment 435156 [details]
Patch

[Yijia Huang]

Created attachment 435162 [details]
Patch

[Yijia Huang]

Created attachment 435164 [details]
Patch

[Yijia Huang]

Created attachment 435168 [details]
Patch

[Keith Miller]

Comment on attachment 435168 [details]
Patch

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

r=me with nit.

> Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:147
> +    auto detect = [&] (const HashMap<MemoryValue*, Value*>& candidates, bool isPreIndexCandidates) {

Nit: I'd change `isPreIndexCandidate` to a lambda which contains the logic to do the canonicalization. I find it to be an anti pattern to pass a bool to a lambda where there is exactly one call with true and false.

[EWS]

Committed r280808 (240378@main): <https://commits.webkit.org/240378@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/81719365>

[Mark Lam]

(In reply to Keith Miller from comment #13)
> Comment on attachment 435168 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435168&action=review
> 
> r=me with nit.
> 
> > Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:147
> > +    auto detect = [&] (const HashMap<MemoryValue*, Value*>& candidates, bool isPreIndexCandidates) {
> 
> Nit: I'd change `isPreIndexCandidate` to a lambda which contains the logic
> to do the canonicalization. I find it to be an anti pattern to pass a bool
> to a lambda where there is exactly one call with true and false.

Why was this not addressed in the landed patch?

[Yijia Huang]

(In reply to Mark Lam from comment #16)
> (In reply to Keith Miller from comment #13)
> > Comment on attachment 435168 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=435168&action=review
> > 
> > r=me with nit.
> > 
> > > Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:147
> > > +    auto detect = [&] (const HashMap<MemoryValue*, Value*>& candidates, bool isPreIndexCandidates) {
> > 
> > Nit: I'd change `isPreIndexCandidate` to a lambda which contains the logic
> > to do the canonicalization. I find it to be an anti pattern to pass a bool
> > to a lambda where there is exactly one call with true and false.
> 
> Why was this not addressed in the landed patch?

Sorry, I will update it in the next patch with the store instruction.

[Mark Lam]

Comment on attachment 435168 [details]
Patch

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

> Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:157
> +                // ***                          -->  newMemory = Load(base, offset)
> +                // ***                               ***

Curious: what is the meaning of the "***" here?

[Yijia Huang]

(In reply to Mark Lam from comment #18)
> Comment on attachment 435168 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435168&action=review
> 
> > Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:157
> > +                // ***                          -->  newMemory = Load(base, offset)
> > +                // ***                               ***
> 
> Curious: what is the meaning of the "***" here?

I was using the ellipsis (...) to indicate any code between them. But it violates the code style, where the format checker requires only one space after the period.

[Mark Lam]

(In reply to Yijia Huang from comment #19)
> (In reply to Mark Lam from comment #18)
> > Comment on attachment 435168 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=435168&action=review
> > 
> > > Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:157
> > > +                // ***                          -->  newMemory = Load(base, offset)
> > > +                // ***                               ***
> > 
> > Curious: what is the meaning of the "***" here?
> 
> I was using the ellipsis (...) to indicate any code between them. But it
> violates the code style, where the format checker requires only one space
> after the period.

Oh, in that case, I would just ignore the style checker because you have a valid use case there.

[Yijia Huang]

(In reply to Mark Lam from comment #20)
> (In reply to Yijia Huang from comment #19)
> > (In reply to Mark Lam from comment #18)
> > > Comment on attachment 435168 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=435168&action=review
> > > 
> > > > Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:157
> > > > +                // ***                          -->  newMemory = Load(base, offset)
> > > > +                // ***                               ***
> > > 
> > > Curious: what is the meaning of the "***" here?
> > 
> > I was using the ellipsis (...) to indicate any code between them. But it
> > violates the code style, where the format checker requires only one space
> > after the period.
> 
> Oh, in that case, I would just ignore the style checker because you have a
> valid use case there.

If I ignore that, would others encounter the checker warning every time they upload a patch because of it?

[Mark Lam]

(In reply to Yijia Huang from comment #21)
> If I ignore that, would others encounter the checker warning every time they
> upload a patch because of it?

Only if they touch that line.