[webkit-reviews] review granted: [Bug 229175] [ARM64] Fix pre-index address mode : [Attachment 436398] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yijia Huang
<yijia_huang at apple.com>'s request for review:
Bug 229175: [ARM64] Fix pre-index address mode
https://bugs.webkit.org/show_bug.cgi?id=229175

Attachment 436398: Patch

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




--- Comment #20 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 436398
  --> https://bugs.webkit.org/attachment.cgi?id=436398
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436398&action=review

r=me with comments

> Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:93
> +		       auto addUsesIter = addUses.find(address);
> +		       if (addUsesIter != addUses.end() &&
addUsesIter->value.size() > 0)
> +			   continue;

I think this is a bit more subtle, and deserves a subtle comment. You may want
to refine, but something like:

```
The goal is to move the Add to this Load/Store. We only move the Add to this
Memory value if we guarantee it dominates all uses.
If there are already other uses of the Add, it guarantees this Memory value
doesn't dominate those uses. This is because we're visiting the program
in pre-order traversal, so we visit this Memory value before all the things it
dominates. So if the Add has other users, we must not dominate those
users. Therefore, this MemoryValue won't be a candidate.
```

An alternative approach could be to just record indices of all users of the
Add. And in your dominance check, you can also add a same-block dominance
check, where the index of the memory value is < index of the Add user if they
are in the same basic block. That algorithm requires more space, but is less
subtle, and more explicit in what it's doing.

> Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:160
> +	       // Convert Pre-Index Load/Store Pattern to the Canonical Form:

both loops inside this loop check for "handledValues.contains(memory)". But
that check should go here before the loops below.

> Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:185
> +		   Value* newAddress =
insertionSet.insert<Value>(insertionIndex, Add, memory->origin(),
address->child(0), address->child(1));

nit: I forgot about the nicer way to do this. We have a clone(Value*) function
on Procedure. So I think the slightly more idiomatic way to do this is:
Value* newAddress = proc.clone(address);

> Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:186
> +		   // update all address references with the newAddress

nit: remove this comment, since it's just saying exactly what the code below is
doing. We avoid these kinds of comments in WebKit, like:

// do X
X

> Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:219
> +	   // This will reset values' indexes if there are any insertions.

I see what you're saying now. I'd just say that after this executes,
memoryToIndex no longer contains up to date information for this block. But
that's ok because we never touch this block again.

> Source/JavaScriptCore/b3/B3ValueKeyInlines.h:41
> +    // The observation is that when both child->index() and offset are 0's,
> +    // HashMap would not accept the ValueKey.

Isn't it sufficient to just do child->index() + 1? Why also offset + 1?