Identify memcpy loops in b3
Created attachment 374999 [details] Patch
This patch does not identify any loops on arm. I think accepting more loops can go in a separate patch later on.
We are getting timeouts on the microbenchmarks, so I will lower the number of iterations after this patch gets reviewed.
Comment on attachment 374999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374999&action=review > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:44 > +static void forwardMemcpy(char* dst, const char* src, size_t size) why not just call memmove? > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:46 > + if (static_cast<size_t>(abs(dst-src)) < size) { "dst-src" => "dst - src" > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:79 > + if (load->type() != Int32) > + return; why? > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:89 > + if (load->opcode() != Load || dstAddr->opcode() != ZExt32) > + return; nit: seems like the load opcode check belongs when you first define it.
Comment on attachment 374999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374999&action=review > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:51 > + std::memcpy(dst, src, size); Why not memmove? This looks wrong. > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:70 > + void reduceMemmove() can you add a comment that explains what you're trying to reduce here in some C like pseudo-code? Or some high level IR example
Comment on attachment 374999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374999&action=review >> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:51 >> + std::memcpy(dst, src, size); > > Why not memmove? This looks wrong. If we prove they do not overlap, then we can just call memcpy. Otherwise, a byte-copy loop does the "wrong thing," while memmove incorrectly does the "right thing." >> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:79 >> + return; > > why? I only wrote tests for int32, since I hardcoded the shift value
Created attachment 375117 [details] Patch
Created attachment 375168 [details] Patch
Comment on attachment 375168 [details] Patch Attachment 375168 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12837003 New failing tests: wasm.yaml/wasm/stress/memcpy-wasm-loop.js.wasm-slow-memory
Created attachment 375178 [details] Patch
(In reply to Justin Michaud from comment #6) > Comment on attachment 374999 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=374999&action=review > > >> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:51 > >> + std::memcpy(dst, src, size); > > > > Why not memmove? This looks wrong. > > If we prove they do not overlap, then we can just call memcpy. Otherwise, a > byte-copy loop does the "wrong thing," while memmove incorrectly does the > "right thing." Right. Makes sense. This is because you proved they're doing a forwards loop. So the user could get this optimization where they overwrite parts of their own buffer. (Where memmove would break those semantics. Tricky!) > > >> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:79 > >> + return; > > > > why? > > I only wrote tests for int32, since I hardcoded the shift value
(In reply to Saam Barati from comment #11) > > > > >> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:79 > > >> + return; > > > > > > why? > > > > I only wrote tests for int32, since I hardcoded the shift value It is fixed now
Comment on attachment 375178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375178&action=review > Source/JavaScriptCore/ChangeLog:9 > + We also update WasmCheckBounds to accept 64-bit values (provided they do not overflow when added to offset) I don't follow. Can you explain why this is needed? > Source/JavaScriptCore/ChangeLog:10 > + in order to allow hoisting these checks to before the memcpy loop. style nit: one too many spaces at the start > Source/JavaScriptCore/ChangeLog:20 > + Similar results are seen on arm64e, but ~16x/13x instead of ~6x/3x. I think you can just say arm64 instead of arm64e > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:99 > + patternMatchedValues.add(store); do this after the branch below. > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:161 > + patternMatchedValues.add(load); style nit: put this above the lambda and after the branch. (Alternatively, just put all these hash table additions in one spot in the end so you don't pay the price when we don't do anything.) > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:173 > + BasicBlock* loopHead = loop->header(), *loopFoot = nullptr; style: two lines of declarations. > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:180 > + // Look for a block that dominates the entrance into the loop to take as our head. Why aren't you just using loop preheader? Is it possible to turn this into a two part phase: 1. Identify if you can transform 2. Transform and do any kind of expensive work to gather metadata You could run ensureLoopPreHeaders in (2) LICM also grabs the preheader. I'd just turn that into a helper. > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:192 > + loopHead = newHead; > + if (!loopHead) > + return; This is just preHeader > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:197 > + for (auto child : block->successors()) { we usually call this successor. > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:199 > + if (!loopInnerBlocks.contains(child.block())) > + loopExits.append(child.block()); I don't find how this control flow analysis is sound in any way. What if you have code like this: ``` i = 0; while (i != size) { if (b) { a[i] = b[i]; i = i + 1; } } ``` I think you want something like control flow equivalent to the real loop header (not what you're calling head, which is really pre header). > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:201 > + for (auto* parent : block->predecessors()) { we usually call this predecessor. > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:212 > + Your analysis is broken on exitsSideways. Nothing currently stresses this as we don't exit sideways in Wasm AFAIK. However, let's just make it sound. We probably just want to bail if we have exitSideways. I wonder if just breaking out the LoopData part of LICM would be helpful for this phase. > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:231 > + if (up->owner == loopHead) > + startingIndex = up; Why not just owner.dominates(loopHead)? > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:257 > + // Find the inner block where we decide to exit, and walk up until we hit a branch. why isn't this just loopFoot > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:259 > + for (BasicBlock* block : loopFoot->predecessors()) { we usually call this "predecessor" > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:274 > + BasicBlock* exitBlock = loopFoot; > + while (branchBlock && branchBlock->numSuccessors() == 1) { > + if (!loopInnerBlocks.contains(branchBlock) || branchBlock->numPredecessors() != 1) > + return; > + exitBlock = branchBlock; > + branchBlock = branchBlock->predecessor(0); > + } > + if (!loopInnerBlocks.contains(branchBlock)) > + return; this is confusing. I'm not sure what you're trying to find. I kinda think what you really want is just control flow equivalent. If your store/load/increment is control flow equivalent to the loop header, then if the loop header executes, your thing is guaranteed to execute. > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:323 > + // Make sure there are no other effectful things happening. It really seems like you want LoopData from LICM :) > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:341 > + effects.readsLocalState = effects.controlDependent = effects.readsPinned = false; put these all on their own line of code > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:350 > + if (loopInnerBlocks.contains(dominator) || escapesBlock) the variable named "dominator" here is backwards. This is like a "dominatee" (I'm not sure what the actual term is) > Source/JavaScriptCore/b3/B3ReduceLoopStrength.h:33 > + I'd add a comment explaining what it's doing. (See. other phase headers for similar comments). > Source/JavaScriptCore/b3/B3ReduceLoopStrength.h:34 > +JS_EXPORT_PRIVATE bool reduceLoopStrength(Procedure&); why exported?
Comment on attachment 375178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375178&action=review >> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:212 >> + > > Your analysis is broken on exitsSideways. Nothing currently stresses this as we don't exit sideways in Wasm AFAIK. However, let's just make it sound. We probably just want to bail if we have exitSideways. > > I wonder if just breaking out the LoopData part of LICM would be helpful for this phase. When we compare against Effects::None(), a value with exitsSideways will fail unless it was pattern-matched. >> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:350 >> + if (loopInnerBlocks.contains(dominator) || escapesBlock) > > the variable named "dominator" here is backwards. This is like a "dominatee" (I'm not sure what the actual term is) I just removed this check, because nothing inside the loop could possibly dominate something outside.
Comment on attachment 375178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375178&action=review >> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:199 >> + loopExits.append(child.block()); > > I don't find how this control flow analysis is sound in any way. What if you have code like this: > > ``` > i = 0; > while (i != size) { > if (b) { > a[i] = b[i]; > i = i + 1; > } > } > ``` > > I think you want something like control flow equivalent to the real loop header (not what you're calling head, which is really pre header). I think https://bugs.webkit.org/show_bug.cgi?id=195991 makes this difficult. Right now, I am just making sure there are no un-matched branches.
Created attachment 375245 [details] Patch
Comment on attachment 375245 [details] Patch Attachment 375245 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12843389 New failing tests: jsc-layout-tests.yaml/js/script-tests/repeat-cached-vm-reentry.js.layout-ftl-no-cjit jsc-layout-tests.yaml/js/script-tests/repeat-cached-vm-reentry.js.layout-ftl-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/array-sort-reentrance.js.layout-ftl-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/repeat-cached-vm-reentry.js.layout
Created attachment 375361 [details] Patch
Failing tests were caused by a dataLog, and should be fixed now
Created attachment 375362 [details] Patch
Comment on attachment 375362 [details] Patch Attachment 375362 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12849631 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases
Comment on attachment 375362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375362&action=review > JSTests/microbenchmarks/memcpy-loop.js:1 > +function doTest(arr1) { It would be nice to skip those tests on 32-bit architectures, since they don't support WASM yet.
Comment on attachment 375362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375362&action=review Some drive by comments. > Source/JavaScriptCore/ChangeLog:12 > + Add a new pass in B3 to identify one type of memcpy loop and replace it with a call to system memcpy. > + We also update WasmCheckBounds to accept 64-bit values (provided they do not overflow when added to offset) > + in order to allow hoisting these checks. We turn the check of the index inside the loop to a check that the > + base + index - 1 (as a 64-bit addition) is in bounds. > + You should explain how you are doing this optimization in the Changelog. > Source/JavaScriptCore/ChangeLog:21 > + Similar results are seen on arm64, but ~16x/13x instead of ~6x/3x. Nice! > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:51 > + if (dst == src || (dst < src && dst + size >= src) || (dst > src && src + size >= dst)) { > + for (size_t i = 0; i < size; ++i) > + dst[i] = src[i]; > + return; > + } why not just call memmove? > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:111 > + if (verbose) > + dataLogLn("Found store/load:", *store); Just as a note, there's a dataLogLnIf(verbose,...) option too. I personally prefer it because I find the ifs distracting. > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:119 > + static_cast<WasmAddrInfo&>(*addr).memoryGPR = value->as<WasmAddressValue>()->pinnedGPR(); Why do you need this? IIRC, there's only one pinned wasmMemoryBaseGPR.
Just for posterity, the main issue I spoke with Justin offline is that this completely breaks Wasm both for bounds checked and signaling memory.
(In reply to Saam Barati from comment #24) > Just for posterity, the main issue I spoke with Justin offline is that this > completely breaks Wasm both for bounds checked and signaling memory. I think the immediate plan is just to match a backwards loop with a single WasmBoundsCheck for the last value. The more extensive future plan is to have some way to have a special Check node which checks the last index is in bounds. If it's not, it replays the effects of the byte copy loop up until the OOB store, and then jumps to the Wasm exception path.
Created attachment 375952 [details] Memcpy respecting load/store size Test program for custom memcpy implementation
Created attachment 375958 [details] Patch
I have ignored Wasm for now, and am just focusing on typed arrays.
Attachment 375958 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:142: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:142: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 2 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Style issues seem to be a bug with the script
Comment on attachment 375958 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375958&action=review > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:51 > +static void forwardMemcpy32(uint32_t* dst, const uint32_t* src, size_t size) can you expose this in a header somewhere and write tests for it that stress all cases. For the purpose of this current review I'm going to assume it's correct. > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:155 > + struct AddrInfo { This shouldn't be virtual > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:176 > + struct WasmAddrInfo : public AddrInfo { > + Value* appendAddr(Procedure& proc, BasicBlock* block, Value* addr) override > + { > + return block->appendNew<WasmAddressValue>(proc, addr->origin(), addr, memoryGPR); > + } > + > + GPRReg memoryGPR; > + }; this is unused. > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:246 > + patternMatchedValues.add(addr->index); what about arrayBase? > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:270 > + Vector<BasicBlock*> loopInnerBlocks; HashSet or SmallPtrSet? This can be big and you search through it many times. Better not to do an N^2 algo here. > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:285 > + for (auto parent : loopHead->predecessors()) { > + if (!loopInnerBlocks.contains(parent)) { > + if (loopPreheader) > + return; > + loopPreheader = parent; > + } > + } can't this just be done by checking loopEntrances.size() after the below loop and ensuring it is in the predecessor list of the header? > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:459 > + memcpy->appendNew<CCallValue>(m_proc, B3::Void, origin, > + memcpy->appendNew<ConstPtrValue>(m_proc, origin, tagCFunctionPtr<void*>(forwardMemcpy32, B3CCallPtrTag)), > + destination->appendAddr(m_proc, memcpy, destination->arrayBase), > + source->appendAddr(m_proc, memcpy, source->arrayBase), > + loopBound); should we bound the effects to the read/write? > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:475 > + Effects effects = value->effects(); > + effects.readsPinned = false; > + if (effects != Effects::none()) { > + if (verbose) > + dataLogLn("Cannot hoist ", *value, " into ", *into, " since it has effects"); > + return false; > + } why readsPinned? > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:484 > + hoistValue(into, child); nit: assert result > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:525 > + } while (m_changed && m_proc.optLevel() >= 2); why are we fix pointing this? I don't think that makes sense, and I don't think detecting one loop will ever allow us to detect another IIUC > Source/JavaScriptCore/dfg/DFGArrayMode.cpp:-287 > - if (node->op() == PutByVal) { > - if (graph.hasExitSite(node->origin.semantic, OutOfBounds) || !isInBounds()) > - return typedArrayResult(withSpeculation(Array::OutOfBounds)); > - } this looks bad. Why was this needed? Seems super wrong to ignore exit sites.
Comment on attachment 375958 [details] Patch Attachment 375958 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12888572 New failing tests: stress/int8-repeat-out-of-bounds.js.default stress/float32-repeat-out-of-bounds.js.no-cjit-validate-phases stress/int8-repeat-out-of-bounds.js.no-cjit-collect-continuously stress/int8-repeat-out-of-bounds.js.no-cjit-validate-phases stress/float32-repeat-out-of-bounds.js.default stress/float32-repeat-out-of-bounds.js.no-llint stress/int8-repeat-out-of-bounds.js.no-llint stress/int8-repeat-out-of-bounds.js.no-ftl stress/int8-repeat-out-of-bounds.js.ftl-no-cjit-validate-sampling-profiler stress/float32-repeat-out-of-bounds.js.ftl-no-cjit-validate-sampling-profiler stress/float32-repeat-out-of-bounds.js.no-ftl stress/float32-repeat-out-of-bounds.js.ftl-no-cjit-b3o0 stress/float32-repeat-out-of-bounds.js.ftl-no-cjit-no-inline-validate stress/float32-repeat-out-of-bounds.js.no-cjit-collect-continuously stress/int8-repeat-out-of-bounds.js.ftl-no-cjit-no-inline-validate stress/int8-repeat-out-of-bounds.js.ftl-no-cjit-b3o0
Created attachment 376222 [details] Patch
https://bugs.webkit.org/show_bug.cgi?id=149886 is the related bug
Attachment 376222 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:143: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:143: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 2 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 376228 [details] Patch
Attachment 376228 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:143: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:143: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 2 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 376228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376228&action=review > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:49 > + * If we would use system memcpy, then it is possibly that we would get a byte copy loop, possibly => possible > Source/JavaScriptCore/runtime/JSObject.h:198 > ALWAYS_INLINE bool putByIndexInline(ExecState* exec, unsigned propertyName, JSValue value, bool shouldThrow) why not pass a VM through to here? > Source/JavaScriptCore/runtime/JSObject.h:360 > + if (canSetIndexQuicklyForTypedArray(vm, i)) > + return true; you should put this inside the switch below for the corresponding indexing type. IIRC, indexing type for typed arrays is blank > Source/JavaScriptCore/runtime/JSObject.h:386 > + bool canSetIndexQuicklyForTypedArray(VM&, unsigned); > + void setIndexQuicklyForTypedArray(VM&, unsigned, JSValue); why not get too? > Source/JavaScriptCore/runtime/JSObjectInlines.h:409 > + if (auto* typedArray = jsCast<JS ## name ## Array *>(this))\ this is wrong. You are looking for jsDynamicCast, which might return null. But why do you need this type check anyways? Is asking class info not enough? Also, instead of loading the class info, I think you can just check the JSType on the object itself. I think that should be faster since it's not a dependent load. You also won't have to thread through VM. > Source/JavaScriptCore/runtime/JSObjectInlines.h:426 > + ASSERT(typedArray);\ this assert is bogus. See above as well for using JSType.
Comment on attachment 376228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376228&action=review > Source/JavaScriptCore/runtime/JSObject.h:437 > + setIndexQuicklyForTypedArray(vm, i, v); let's do this under the proper indexing type (of blank IIRC)
Created attachment 376412 [details] Patch
Attachment 376412 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:143: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:143: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 2 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 376412 [details] Patch Attachment 376412 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12919793 Number of test failures exceeded the failure limit.
Created attachment 376416 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 376412 [details] Patch Attachment 376412 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12919888 Number of test failures exceeded the failure limit.
Created attachment 376423 [details] Archive of layout-test-results from ews114 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 376412 [details] Patch Attachment 376412 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12919893 New failing tests: stress/arguments-bizarre-behavior.js.ftl-no-cjit-no-put-stack-validate ChakraCore.yaml/ChakraCore/test/Array/array_splice_double.js.default stress/array-lastindexof-cached-length.js.dfg-eager-no-cjit-validate stress/typedarray-map.js.ftl-eager stress/proxy-array-prototype-methods.js.no-cjit-validate-phases ChakraCore.yaml/ChakraCore/test/typedarray/UInt16Array2.js.default mozilla-tests.yaml/js1_6/Array/regress-304828.js.mozilla-llint stress/typedarray-fill.js.no-llint stress/arguments-bizarre-behavior.js.ftl-eager-no-cjit stress/array-join-on-strings-need-overflow-checks.js.no-ftl stress/arguments-copy-register-array-backing-store.js.dfg-eager mozilla-tests.yaml/js1_6/Array/regress-304828.js.mozilla stress/array-indexof-cached-length.js.default stress/typedarray-fill.js.default stress/array-indexof-cached-length.js.no-ftl stress/array-join-on-strings-need-overflow-checks.js.dfg-eager-no-cjit-validate stress/array-indexof-cached-length.js.ftl-no-cjit-validate-sampling-profiler stress/dfg-create-arguments-inline-alloc.js.no-ftl microbenchmarks/emscripten-cube2hash.js.ftl-no-cjit-no-inline-validate es6.yaml/es6/Proxy_internal_set_calls_Array.prototype.unshift.js.default ChakraCore.yaml/ChakraCore/test/Function/sameNamePara.js.default stress/typedarray-map.js.no-ftl stress/array-lastindexof-cached-length.js.no-llint stress/array-prototype-splice-making-typed-array.js.no-llint stress/dfg-create-arguments-inline-alloc.js.ftl-no-cjit-no-put-stack-validate stress/proxy-array-prototype-methods.js.dfg-maximal-flush-validate-no-cjit microbenchmarks/emscripten-cube2hash.js.ftl-eager-no-cjit stress/array-join-on-strings-need-overflow-checks.js.ftl-eager stress/rest-parameter-inlined.js.dfg-eager-no-cjit-validate stress/arguments-bizarre-behavior.js.no-cjit-collect-continuously mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases stress/arguments-copy-register-array-backing-store.js.bytecode-cache jsc-layout-tests.yaml/js/script-tests/function-dot-arguments.js.layout ChakraCore.yaml/ChakraCore/test/typedarray/Int32Array2.js.default es6.yaml/es6/Proxy_internal_set_calls_Array.prototype.shift.js.default stress/dfg-create-arguments-inline-alloc.js.dfg-maximal-flush-validate-no-cjit stress/array-prototype-splice-making-typed-array.js.no-ftl stress/array-lastindexof-cached-length.js.ftl-no-cjit-small-pool stress/array-join-on-strings-need-overflow-checks.js.ftl-eager-no-cjit stress/proxy-array-prototype-methods.js.mini-mode stress/typedarray-fill.js.ftl-eager stress/arguments-bizarre-behavior.js.default stress/array-prototype-splice-making-typed-array.js.default stress/typedarray-from.js.dfg-eager stress/typedarray-map.js.dfg-eager stress/array-prototype-splice-making-typed-array.js.no-cjit-collect-continuously stress/array-prototype-splice-making-typed-array.js.no-cjit-validate-phases stress/array-lastindexof-cached-length.js.no-ftl stress/dfg-create-arguments-inline-alloc.js.mini-mode stress/typedarray-from.js.ftl-eager stress/typedarray-fill.js.ftl-no-cjit-small-pool stress/arguments-copy-register-array-backing-store.js.no-llint stress/typedarray-map.js.ftl-eager-no-cjit stress/rest-parameter-inlined.js.ftl-no-cjit-no-inline-validate jsc-layout-tests.yaml/js/script-tests/array-functions-non-arrays.js.layout-ftl-eager-no-cjit ChakraCore.yaml/ChakraCore/test/es5/defineIndexProperty.js.default mozilla-tests.yaml/js1_6/Array/regress-304828.js.mozilla-baseline mozilla-tests.yaml/js1_6/Array/regress-304828.js.mozilla-no-ftl jsc-layout-tests.yaml/js/script-tests/function-dot-arguments.js.layout-ftl-no-cjit stress/dfg-create-arguments-inline-alloc.js.dfg-eager-no-cjit-validate stress/arguments-copy-register-array-backing-store.js.ftl-no-cjit-b3o0 stress/array-indexof-cached-length.js.ftl-no-cjit-no-inline-validate stress/typedarray-functions-with-neutered.js.ftl-eager stress/array-prototype-splice-making-typed-array.js.mini-mode stress/arguments-copy-register-array-backing-store.js.no-cjit-validate-phases stress/proxy-array-prototype-methods.js.dfg-eager stress/typedarray-fill.js.dfg-maximal-flush-validate-no-cjit stress/rest-parameter-inlined.js.bytecode-cache stress/arguments-bizarre-behavior.js.ftl-no-cjit-small-pool stress/array-indexof-cached-length.js.mini-mode stress/proxy-array-prototype-methods.js.ftl-eager-no-cjit stress/arguments-copy-register-array-backing-store.js.ftl-no-cjit-no-put-stack-validate stress/array-join-on-strings-need-overflow-checks.js.no-cjit-collect-continuously stress/typedarray-map.js.no-llint stress/array-prototype-splice-making-typed-array.js.ftl-no-cjit-no-inline-validate jsc-layout-tests.yaml/js/script-tests/function-dot-arguments.js.layout-no-ftl stress/array-prototype-splice-making-typed-array.js.bytecode-cache stress/array-indexof-cached-length.js.dfg-eager stress/array-join-on-strings-need-overflow-checks.js.ftl-no-cjit-no-inline-validate stress/array-indexof-cached-length.js.bytecode-cache mozilla-tests.yaml/js1_6/Array/regress-304828.js.mozilla-dfg-eager-no-cjit-validate-phases stress/dfg-create-arguments-inline-alloc.js.ftl-no-cjit-b3o0 stress/arguments-copy-register-array-backing-store.js.ftl-eager-no-cjit stress/proxy-array-prototype-methods.js.default es6.yaml/es6/Proxy_internal_get_calls_Array.prototype_iteration_methods.js.default stress/proxy-array-prototype-methods.js.ftl-eager-no-cjit-b3o1 stress/arguments-bizarre-behavior.js.ftl-eager stress/arguments-bizarre-behavior.js.ftl-no-cjit-no-inline-validate stress/rest-parameter-inlined.js.no-cjit-validate-phases stress/array-lastindexof-cached-length.js.ftl-no-cjit-no-inline-validate ChakraCore.yaml/ChakraCore/test/typedarray/Int8Array2.js.default microbenchmarks/emscripten-cube2hash.js.no-cjit-validate-phases stress/arguments-copy-register-array-backing-store.js.ftl-no-cjit-no-inline-validate stress/typedarray-map.js.dfg-eager-no-cjit-validate microbenchmarks/emscripten-cube2hash.js.ftl-no-cjit-b3o0 stress/proxy-array-prototype-methods.js.ftl-no-cjit-no-inline-validate stress/proxy-array-prototype-methods.js.dfg-eager-no-cjit-validate stress/array-prototype-splice-making-typed-array.js.ftl-no-cjit-validate-sampling-profiler jsc-layout-tests.yaml/js/script-tests/array-functions-non-arrays.js.layout-dfg-eager-no-cjit stress/typedarray-map.js.default stress/array-indexof-cached-length.js.no-cjit-collect-continuously stress/typedarray-from.js.ftl-eager-no-cjit stress/proxy-array-prototype-methods.js.ftl-no-cjit-validate-sampling-profiler stress/array-lastindexof-cached-length.js.ftl-eager-no-cjit stress/rest-parameter-inlined.js.default jsc-layout-tests.yaml/js/script-tests/function-dot-arguments.js.layout-dfg-eager-no-cjit stress/dfg-create-arguments-inline-alloc.js.no-cjit-collect-continuously stress/arguments-copy-register-array-backing-store.js.dfg-maximal-flush-validate-no-cjit stress/typedarray-map.js.bytecode-cache stress/array-prototype-splice-making-typed-array.js.dfg-eager-no-cjit-validate stress/array-indexof-cached-length.js.ftl-no-cjit-no-put-stack-validate microbenchmarks/emscripten-cube2hash.js.dfg-maximal-flush-validate-no-cjit stress/typedarray-functions-with-neutered.js.dfg-eager stress/array-prototype-splice-making-typed-array.js.ftl-no-cjit-b3o0 stress/array-indexof-cached-length.js.dfg-maximal-flush-validate-no-cjit jsc-layout-tests.yaml/js/script-tests/array-functions-non-arrays.js.layout stress/array-join-on-strings-need-overflow-checks.js.no-llint stress/typedarray-fill.js.no-ftl stress/array-lastindexof-cached-length.js.dfg-maximal-flush-validate-no-cjit stress/array-lastindexof-cached-length.js.no-cjit-validate-phases stress/arguments-copy-register-array-backing-store.js.ftl-eager stress/dfg-create-arguments-inline-alloc.js.ftl-eager jsc-layout-tests.yaml/js/script-tests/function-dot-arguments.js.layout-no-cjit stress/array-lastindexof-cached-length.js.ftl-no-cjit-b3o0 stress/array-indexof-cached-length.js.no-cjit-validate-phases stress/proxy-array-prototype-methods.js.ftl-eager stress/rest-parameter-inlined.js.mini-mode stress/arguments-bizarre-behavior.js.dfg-maximal-flush-validate-no-cjit stress/typedarray-from.js.no-ftl stress/arguments-copy-register-array-backing-store.js.mini-mode jsc-layout-tests.yaml/js/script-tests/function-dot-arguments.js.layout-no-llint stress/rest-parameter-inlined.js.ftl-no-cjit-no-put-stack-validate stress/dfg-create-arguments-inline-alloc.js.ftl-eager-no-cjit stress/array-join-on-strings-need-overflow-checks.js.dfg-maximal-flush-validate-no-cjit stress/arguments-copy-register-array-backing-store.js.dfg-eager-no-cjit-validate stress/array-lastindexof-cached-length.js.no-cjit-collect-continuously stress/array-lastindexof-cached-length.js.default stress/arguments-bizarre-behavior.js.ftl-no-cjit-validate-sampling-profiler stress/dfg-create-arguments-inline-alloc.js.bytecode-cache stress/typedarray-map.js.no-cjit-collect-continuously microbenchmarks/emscripten-cube2hash.js.no-cjit-collect-continuously stress/arguments-bizarre-behavior.js.no-cjit-validate-phases stress/typedarray-from.js.ftl-no-cjit-validate-sampling-profiler stress/array-join-on-strings-need-overflow-checks.js.ftl-no-cjit-b3o0 stress/proxy-array-prototype-methods.js.no-llint ChakraCore.yaml/ChakraCore/test/typedarray/UInt32Array2.js.default ChakraCore.yaml/ChakraCore/test/Array/reverse2.js.default stress/dfg-create-arguments-inline-alloc.js.no-cjit-validate-phases stress/arguments-copy-register-array-backing-store.js.no-ftl stress/typedarray-fill.js.ftl-no-cjit-b3o0 stress/proxy-array-prototype-methods.js.ftl-no-cjit-b3o0 stress/array-join-on-strings-need-overflow-checks.js.no-cjit-validate-phases stress/proxy-array-prototype-methods.js.no-ftl stress/array-prototype-splice-making-typed-array.js.dfg-eager stress/typedarray-from.js.no-llint stress/typedarray-from.js.no-cjit-collect-continuously es6.yaml/es6/Proxy_internal_deleteProperty_calls_Array.prototype.splice.js.default stress/typedarray-from.js.no-cjit-validate-phases jsc-layout-tests.yaml/js/script-tests/array-functions-non-arrays.js.layout-no-ftl stress/arguments-bizarre-behavior.js.dfg-eager stress/array-indexof-cached-length.js.ftl-no-cjit-small-pool ChakraCore.yaml/ChakraCore/test/typedarray/Int16Array2.js.default stress/array-lastindexof-cached-length.js.dfg-eager stress/typedarray-fill.js.mini-mode stress/arguments-copy-register-array-backing-store.js.default stress/arguments-bizarre-behavior.js.mini-mode stress/typedarray-functions-with-neutered.js.ftl-no-cjit-small-pool es6.yaml/es6/Proxy_internal_set_calls_Array.prototype.splice.js.default stress/arguments-bizarre-behavior.js.ftl-no-cjit-b3o0 stress/array-prototype-splice-making-typed-array.js.ftl-no-cjit-no-put-stack-validate stress/typedarray-from.js.ftl-eager-no-cjit-b3o1 stress/rest-parameter-inlined.js.dfg-maximal-flush-validate-no-cjit es6.yaml/es6/Proxy_internal_deleteProperty_calls_Array.prototype.unshift.js.default stress/array-indexof-cached-length.js.no-llint ChakraCore.yaml/ChakraCore/test/Array/array_splice.js.default ChakraCore.yaml/ChakraCore/test/typedarray/UInt8Array2.js.default stress/arguments-copy-register-array-backing-store.js.ftl-eager-no-cjit-b3o1 stress/dfg-create-arguments-inline-alloc.js.no-llint microbenchmarks/emscripten-cube2hash.js.no-llint stress/arguments-bizarre-behavior.js.dfg-eager-no-cjit-validate stress/typedarray-from.js.ftl-no-cjit-b3o0 stress/typedarray-map.js.ftl-no-cjit-validate-sampling-profiler es6.yaml/es6/Proxy_internal_get_calls_Array.prototype.shift.js.default es6.yaml/es6/Proxy_internal_deleteProperty_calls_Array.prototype.shift.js.default stress/proxy-array-prototype-methods.js.ftl-no-cjit-small-pool stress/typedarray-fill.js.no-cjit-collect-continuously stress/arguments-bizarre-behavior.js.ftl-eager-no-cjit-b3o1 stress/typedarray-fill.js.ftl-no-cjit-validate-sampling-profiler ChakraCore.yaml/ChakraCore/test/typedarray/Float64Array2.js.default microbenchmarks/emscripten-cube2hash.js.ftl-no-cjit-validate-sampling-profiler stress/proxy-array-prototype-methods.js.no-cjit-collect-continuously stress/array-join-on-strings-need-overflow-checks.js.ftl-eager-no-cjit-b3o1 stress/arguments-copy-register-array-backing-store.js.ftl-no-cjit-small-pool stress/dfg-create-arguments-inline-alloc.js.dfg-eager stress/typedarray-map.js.mini-mode stress/rest-parameter-inlined.js.ftl-no-cjit-small-pool stress/rest-parameter-inlined.js.ftl-no-cjit-b3o0 microbenchmarks/emscripten-cube2hash.js.no-ftl stress/array-lastindexof-cached-length.js.mini-mode stress/typedarray-from.js.dfg-maximal-flush-validate-no-cjit stress/typedarray-from.js.ftl-no-cjit-small-pool es6.yaml/es6/Proxy_internal_get_calls_Array.prototype.splice.js.default stress/rest-parameter-inlined.js.ftl-eager-no-cjit-b3o1 stress/typedarray-from.js.ftl-no-cjit-no-put-stack-validate ChakraCore.yaml/ChakraCore/test/Array/array_sort2.js.default microbenchmarks/emscripten-cube2hash.js.bytecode-cache mozilla-tests.yaml/js1_6/Array/regress-304828.js.mozilla-ftl-eager-no-cjit-validate-phases stress/arguments-copy-register-array-backing-store.js.no-cjit-collect-continuously ChakraCore.yaml/ChakraCore/test/typedarray/Float32Array2.js.default stress/typedarray-from.js.bytecode-cache stress/typedarray-map.js.dfg-maximal-flush-validate-no-cjit stress/typedarray-from.js.dfg-eager-no-cjit-validate stress/typedarray-map.js.ftl-no-cjit-no-put-stack-validate stress/dfg-create-arguments-inline-alloc.js.ftl-no-cjit-no-inline-validate stress/array-join-on-strings-need-overflow-checks.js.ftl-no-cjit-validate-sampling-profiler stress/array-prototype-splice-making-typed-array.js.ftl-eager-no-cjit stress/dfg-create-arguments-inline-alloc.js.default stress/rest-parameter-inlined.js.ftl-no-cjit-validate-sampling-profiler stress/typedarray-fill.js.dfg-eager ChakraCore.yaml/ChakraCore/test/Array/toLocaleString.js.default stress/rest-parameter-inlined.js.ftl-eager-no-cjit stress/array-indexof-cached-length.js.ftl-eager stress/array-join-on-strings-need-overflow-checks.js.ftl-no-cjit-no-put-stack-validate jsc-layout-tests.yaml/js/script-tests/array-functions-non-arrays.js.layout-no-cjit stress/array-join-on-strings-need-overflow-checks.js.dfg-eager microbenchmarks/emscripten-cube2hash.js.dfg-eager-no-cjit-validate stress/array-prototype-splice-making-typed-array.js.ftl-eager-no-cjit-b3o1 stress/dfg-create-arguments-inline-alloc.js.ftl-eager-no-cjit-b3o1 stress/rest-parameter-inlined.js.no-llint stress/array-join-on-strings-need-overflow-checks.js.bytecode-cache stress/typedarray-fill.js.no-cjit-validate-phases stress/arguments-bizarre-behavior.js.no-ftl stress/typedarray-map.js.ftl-no-cjit-no-inline-validate stress/array-lastindexof-cached-length.js.ftl-eager stress/array-prototype-splice-making-typed-array.js.ftl-no-cjit-small-pool stress/dfg-create-arguments-inline-alloc.js.ftl-no-cjit-small-pool stress/typedarray-fill.js.bytecode-cache stress/rest-parameter-inlined.js.no-ftl stress/array-lastindexof-cached-length.js.ftl-eager-no-cjit-b3o1 stress/arguments-bizarre-behavior.js.bytecode-cache stress/array-lastindexof-cached-length.js.ftl-no-cjit-no-put-stack-validate stress/array-prototype-splice-making-typed-array.js.dfg-maximal-flush-validate-no-cjit stress/array-indexof-cached-length.js.ftl-no-cjit-b3o0 stress/proxy-array-prototype-methods.js.bytecode-cache microbenchmarks/emscripten-cube2hash.js.ftl-eager-no-cjit-b3o1 microbenchmarks/emscripten-cube2hash.js.mini-mode microbenchmarks/emscripten-cube2hash.js.dfg-eager stress/rest-parameter-inlined.js.dfg-eager stress/typedarray-map.js.ftl-no-cjit-small-pool stress/arguments-copy-register-array-backing-store.js.ftl-no-cjit-validate-sampling-profiler stress/rest-parameter-inlined.js.no-cjit-collect-continuously stress/array-indexof-cached-length.js.ftl-eager-no-cjit-b3o1 stress/proxy-array-prototype-methods.js.ftl-no-cjit-no-put-stack-validate jsc-layout-tests.yaml/js/script-tests/function-dot-arguments.js.layout-ftl-eager-no-cjit stress/array-lastindexof-cached-length.js.ftl-no-cjit-validate-sampling-profiler stress/arguments-bizarre-behavior.js.no-llint microbenchmarks/emscripten-cube2hash.js.default microbenchmarks/emscripten-cube2hash.js.ftl-eager stress/array-indexof-cached-length.js.dfg-eager-no-cjit-validate stress/array-prototype-splice-making-typed-array.js.ftl-eager stress/typedarray-fill.js.ftl-no-cjit-no-inline-validate stress/array-join-on-strings-need-overflow-checks.js.default microbenchmarks/emscripten-cube2hash.js.ftl-no-cjit-no-put-stack-validate stress/typedarray-fill.js.ftl-eager-no-cjit stress/typedarray-from.js.ftl-no-cjit-no-inline-validate stress/array-join-on-strings-need-overflow-checks.js.ftl-no-cjit-small-pool stress/array-lastindexof-cached-length.js.bytecode-cache stress/typedarray-from.js.mini-mode stress/dfg-create-arguments-inline-alloc.js.ftl-no-cjit-validate-sampling-profiler microbenchmarks/emscripten-cube2hash.js.ftl-no-cjit-small-pool stress/typedarray-map.js.ftl-eager-no-cjit-b3o1 stress/rest-parameter-inlined.js.ftl-eager ChakraCore.yaml/ChakraCore/test/Array/join2.js.default stress/array-join-on-strings-need-overflow-checks.js.mini-mode jsc-layout-tests.yaml/js/script-tests/array-functions-non-arrays.js.layout-no-llint stress/typedarray-from.js.default stress/typedarray-fill.js.ftl-eager-no-cjit-b3o1 stress/typedarray-map.js.ftl-no-cjit-b3o0 stress/typedarray-fill.js.ftl-no-cjit-no-put-stack-validate jsc-layout-tests.yaml/js/script-tests/array-functions-non-arrays.js.layout-ftl-no-cjit stress/array-indexof-cached-length.js.ftl-eager-no-cjit stress/typedarray-fill.js.dfg-eager-no-cjit-validate stress/typedarray-map.js.no-cjit-validate-phases
Comment on attachment 376412 [details] Patch Attachment 376412 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12920082 Number of test failures exceeded the failure limit.
Created attachment 376433 [details] Archive of layout-test-results from ews210 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 376543 [details] Patch
Attachment 376543 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:143: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:143: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 2 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 376570 [details] Patch
Attachment 376570 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:143: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:143: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:143: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:143: __attribute__ is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 4 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 376570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376570&action=review > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:147 > +void __attribute__ ((optnone)) fastForwardCopy32(uint32_t* dst, const uint32_t* src, size_t size) > +{ > + for (size_t i = 0; i < size; ++i) > + dst[i] = src[i]; > +} why would this ever be reached? I think we should skip this until we enable this on non x86 > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:176 > + void reduceByteCopyLoopsToMemcpy() can we file some bugs on enhancing this phase? - to Wsam - More than elemSize == 4 - Other loop kinds > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:195 > + auto extractArrayInfo = [&] (Value* value) -> std::unique_ptr<AddrInfo> { nit: why unique_ptr? Why not just optional? That way it'll just be stack allocated by LLVM > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:254 > + auto loop = m_proc.naturalLoops().innerMostLoopOf(m_block); you're not doing a null check here. Please add a test. This should be an insta crash. > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:263 > + for (unsigned i = 0; i < loop->size(); ++i) > + loopInnerBlocks.add(loop->at(i)); this should contain the header, right? > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:266 > + if (!loopInnerBlocks.contains(load->owner)) > + return; can you assert that it contains store->owner just for completeness? > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:334 > + if (m_proc.dominators().dominates(up->owner, loopHead)) > + startingIndex = up; don't we want strictly dominates here? Could be worth a test too. I believe if up->owner == loopHead, we'll just always store at index 0? > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:354 > + if (!startingIndex->isInt(0)) > + return; style nit: seems like a weird place to put this line given you check updateIndex up above > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:447 > + loopBound); where do we ever ensure that loopBound can be hoisted from the loop? Don't we need to do that? What if loopBound is defined in the loop? Seems wrong to do this in such a scenario. Seems like it'd be easy to add a test where loopBound is a constant defined in the loop and you generate invalid SSA here. > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:482 > +#if CPU(X86_64) can we file a bug to enable this on arm64?
Created attachment 376724 [details] Patch
Created attachment 376806 [details] Patch
Comment on attachment 376806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376806&action=review r=me Can you also file a bug to make this work with Wasm again? > Source/JavaScriptCore/ChangeLog:9 > + that will not cause GC tearing and have the correct behaviour when overlapping regions are passed in. do you have tests to ensure overlapping works as expected? > Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:446 > + effects.readsPinned = true; > + effects.writesPinned = true; Not sure this is quite true. > JSTests/microbenchmarks/memcpy-wasm.js:7 > + you should return early from these Wasm tests if WebAssembly is not enabled. I believe we have some WK clients that don't enable it.
Comment on attachment 376806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376806&action=review >> Source/JavaScriptCore/ChangeLog:9 >> + that will not cause GC tearing and have the correct behaviour when overlapping regions are passed in. > > do you have tests to ensure overlapping works as expected? Yes, in testb3, overlap=true
Created attachment 376819 [details] Patch
Comment on attachment 376819 [details] Patch Clearing flags on attachment: 376819 Committed r248938: <https://trac.webkit.org/changeset/248938>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54544434>