Bug 200181 – Identify memcpy loops in b3

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

[Justin Michaud]

Identify memcpy loops in b3

[Justin Michaud]

Created attachment 374999 [details]
Patch

[Justin Michaud]

This patch does not identify any loops on arm. I think accepting more loops can go in a separate patch later on.

[Justin Michaud]

We are getting timeouts on the microbenchmarks, so I will lower the number of iterations after this patch gets reviewed.

[Saam Barati]

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.

[Saam Barati]

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

[Justin Michaud]

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

[Justin Michaud]

Created attachment 375117 [details]
Patch

[Justin Michaud]

Created attachment 375168 [details]
Patch

[EWS Watchlist]

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

[Justin Michaud]

Created attachment 375178 [details]
Patch

[Saam Barati]

(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

[Justin Michaud]

(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

[Saam Barati]

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?

[Justin Michaud]

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.

[Justin Michaud]

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.

[Justin Michaud]

Created attachment 375245 [details]
Patch

[EWS Watchlist]

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

[Justin Michaud]

Created attachment 375361 [details]
Patch

[Justin Michaud]

Failing tests were caused by a dataLog, and should be fixed now

[Justin Michaud]

Created attachment 375362 [details]
Patch

[EWS Watchlist]

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

[Caio Lima]

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.

[Keith Miller]

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.

[Saam Barati]

Just for posterity, the main issue I spoke with Justin offline is that this completely breaks Wasm both for bounds checked and signaling memory.

[Saam Barati]

(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.

[Justin Michaud]

Created attachment 375952 [details]
Memcpy respecting load/store size

Test program for custom memcpy implementation

[Justin Michaud]

Created attachment 375958 [details]
Patch

[Justin Michaud]

I have ignored Wasm for now, and am just focusing on typed arrays.

[EWS Watchlist]

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.

[Justin Michaud]

Style issues seem to be a bug with the script

[Saam Barati]

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.

[EWS Watchlist]

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

[Justin Michaud]

Created attachment 376222 [details]
Patch

[Justin Michaud]

https://bugs.webkit.org/show_bug.cgi?id=149886 is the related bug

[EWS Watchlist]

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.

[Justin Michaud]

Created attachment 376228 [details]
Patch

[EWS Watchlist]

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.

[Saam Barati]

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.

[Saam Barati]

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)

[Justin Michaud]

Created attachment 376412 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Justin Michaud]

Created attachment 376543 [details]
Patch

[EWS Watchlist]

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.

[Justin Michaud]

Created attachment 376570 [details]
Patch

[EWS Watchlist]

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.

[Saam Barati]

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?

[Justin Michaud]

Created attachment 376724 [details]
Patch

[Justin Michaud]

Created attachment 376806 [details]
Patch

[Saam Barati]

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.

[Justin Michaud]

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

[Justin Michaud]

Created attachment 376819 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 376819 [details]
Patch

Clearing flags on attachment: 376819

Committed r248938: <https://trac.webkit.org/changeset/248938>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54544434>