Summary: | [ARM64] Clean up and fix Pre/Post-Indexed Address Mode to Air for ARM64 (Load Instruction) | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yijia Huang <yijia_huang> | ||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Yijia Huang <yijia_huang> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Attachments: |
|
Description
Yijia Huang
2021-08-06 20:54:36 PDT
Created attachment 435116 [details]
Patch
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. Created attachment 435137 [details]
Patch
(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. Created attachment 435139 [details]
Patch
Created attachment 435142 [details]
Patch
Created attachment 435143 [details]
Patch
Created attachment 435148 [details]
Patch
Created attachment 435156 [details]
Patch
Created attachment 435162 [details]
Patch
Created attachment 435164 [details]
Patch
Created attachment 435168 [details]
Patch
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. 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]. (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? (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. 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? (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. (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. (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? (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. |