[webkit-reviews] review denied: [Bug 228781] [ARM64] Add pattern matching for Load/Store Pair : [Attachment 435170] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Yijia Huang
<yijia_huang at apple.com>'s request for review:
Bug 228781: [ARM64] Add pattern matching for Load/Store Pair
https://bugs.webkit.org/show_bug.cgi?id=228781

Attachment 435170: Patch

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




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

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

Would be good to add some tests for the bugs I found.

> Source/JavaScriptCore/ChangeLog:21
> +	       memory2 = Load(base, offset + bytes)
> +	   Or:
> +	       memory1 = Load(base, offset + bytes)

Worth specifying what "bytes" is here. I'm assuming 4/8 for 32/64 bit memory
ops

> Source/JavaScriptCore/b3/B3CanonicalizeLoadStorePair.cpp:60
> +    IndexSet<Value*> ignoredValues;

name nit: ignoredValues => alreadyHandledValues

> Source/JavaScriptCore/b3/B3CanonicalizeLoadStorePair.cpp:65
> +    auto tryAddLoadStorePairCandidates = [&] (Value* value) {

I like to write code like this by having the lambda be in the loop it's called
from. I think you can also make index local to the loop then, too, instead of
defining it above.

> Source/JavaScriptCore/b3/B3CanonicalizeLoadStorePair.cpp:70
> +	       //     ***

I like "..." over "***"

> Source/JavaScriptCore/b3/B3CanonicalizeLoadStorePair.cpp:77
> +	       MemoryValue* memory2 = value->as<MemoryValue>();
> +	       Value* base2 = memory2->lastChild();

name nit: I'd name this "memory" and "base", and in the loop below in
tryAddLoadPairCandidates, call the other variable "memory2"

> Source/JavaScriptCore/b3/B3CanonicalizeLoadStorePair.cpp:114
> +		   if (value2->type() != Int32 && value2->type() != Int64)
> +		       return;
> +		   if (!index)
> +		       return;
> +		   Value* before = block->at(index - 1);
> +		   if (before->opcode() != Store)
> +		       return;

See comment below. I feel like this should be doing code motion to move stores
to be adjacent when possible.

> Source/JavaScriptCore/b3/B3CanonicalizeLoadStorePair.cpp:165
> +	   if (ignoredValues.contains(memory1) ||
ignoredValues.contains(memory2) || !controlEquivalent(memory1, memory2))
> +	       continue;

This isn't a sufficient test for soundness. Imagine you have:

load(p)
store(p + 8)
load(p + 8)

This program will be turned into:
load(p)
load(p + 8)
store(p + 8)
which is wrong.

> Source/JavaScriptCore/b3/B3CanonicalizeLoadStorePair.cpp:208
> +    for (auto pair : storePairCandidates) {
> +	   // ***						   newMemory2 =
Store(value2, base, offset)
> +	   // memory1 = Store(value1, base, offset + bytes) --->   memory1 =
Store(value1, base, offset + bytes)
> +	   // memory2 = Store(value2, base, offset)		   memory2 =
Identity(newMemory2)
> +	   MemoryValue* memory1 = pair.key;
> +	   MemoryValue* memory2 = pair.value;
> +	   Value* value2 = memory2->child(0);
> +	   Value* base2 = memory2->child(1);
> +	   MemoryValue* newMemory2 =
insertionSet.insert<MemoryValue>(valueIndexInBasicBlock(memory1), Store,
memory1->origin(), value2, base2);
> +	   newMemory2->setOffset(memory2->offset());
> +	   memory2->replaceWithIdentity(newMemory2);
> +
> +	   insertionSet.execute(memory1->owner);
> +    }

I don't understand how this works at all. Stores don't produce values.

Why isn't store just doing the same thing as loads, and just doing code motion
on the stores so they're next to each other? I don't get the point of just
looking for adjacent values. Shouldn't the purpose of this canonicalization be
to produce adjacent stores when sound?

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2639
> +		   if (m_locked.contains(memory1) || m_locked.contains(base1))

I don't think base1 can be locked, right? Since base1 == base2, and if we're
generating code for memory2, base2 has to not be locked.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:3505
> +	       auto tryAppendStorePair = [&] () -> bool {

some of this detection is similar to the load case. Can we have a helper that
detects if it's legal to turn into load/store pair?

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:3523
> +		   MemoryValue* memory2 = m_value->as<MemoryValue>();
> +		   Value* value2 = memory2->child(0);
> +		   Air::Opcode opcode = tryOpcodeForType(StorePair32,
StorePair64, value2->type());
> +		   if (!isValidForm(opcode, Arg::Tmp, Arg::Tmp, Arg::Addr) ||
!m_index)
> +		       return false;
> +		   Value* before = m_block->at(m_index - 1);
> +		   if (before->opcode() != Store)
> +		       return false;
> +
> +		   MemoryValue* memory1 = before->as<MemoryValue>();
> +		   Value* value1 = memory1->child(0);
> +		   Value* base1 = memory1->child(1);
> +		   Value* base2 = memory2->child(1);
> +		   Value::OffsetType offset1 = memory1->offset();
> +		   Value::OffsetType offset2 = memory2->offset();
> +		   Value::OffsetType bytes = opcode == StorePair32 ? 4 : 8;
> +		   if (value1->type() != value2->type() || base1 != base2 ||
offset2 - offset1 != bytes)
> +		       return false;

Everything up to here seems essentially the same, maybe except for checking the
type of the values, but you can easily add a branch for that.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:3524
> +		   if (m_locked.contains(memory1) || m_locked.contains(value1)
|| m_locked.contains(value2) || m_locked.contains(base1))

similar comment here about checking about the locked base