...
Created attachment 434407 [details] Patch
Created attachment 434731 [details] Patch
Created attachment 434769 [details] Patch
Created attachment 434792 [details] Patch
Created attachment 434818 [details] Patch
Created attachment 434820 [details] Patch
<rdar://problem/81500926>
Created attachment 435056 [details] Patch
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
Created attachment 435124 [details] Patch
Created attachment 435238 [details] Patch
Created attachment 435261 [details] Patch
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].
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.