[webkit-reviews] review denied: [Bug 200181] Identify memcpy loops in b3 : [Attachment 375178] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 200181: Identify memcpy loops in b3
https://bugs.webkit.org/show_bug.cgi?id=200181

Attachment 375178: Patch

https://bugs.webkit.org/attachment.cgi?id=375178&action=review




--- Comment #13 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 375178
  --> https://bugs.webkit.org/attachment.cgi?id=375178
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?