[ARM64] Fix pre-index address mode

https://bugs.webkit.org/show_bug.cgi?id=229175

Reviewed by Saam Barati.

This patch fixes the canonicalization phase for pre/post-increment address mode
due to the potential bugs commented on in the previous patch
https://bugs.webkit.org/show_bug.cgi?id=228538. And this patch removed the
temporary fix in https://bugs.webkit.org/show_bug.cgi?id=229211.

Previously, the pre-index address mode for Load instruction convert the pattern
to the canonical form like this:

    address = Add(base, offset)       address = Add(base, offset)
    ...                          -->  newMemory = Load(base, offset)
    ...                               ...
    memory = Load(base, offset)       memory = Identity(newMemory)

which is wrong. Assume "..." contains a store to a memory location that aliases for address:

    address = Add(base, offset)       address = Add(base, offset)
    ...                          -->  newMemory = Load(base, offset)
    ...                               ...
    Store(value1, address)            Store(value1, address)
    memory = Load(base, offset)       memory = Identity(newMemory)

The loaded value should always be value1 which is not true after the conversion.
So, moving the load above the store is semantically incorrect because it's not identical to
the behavior of the original program. In this case, maybe we should apply alias analysis to
detect the violations of reference updating.

To solve this problem, we moves the address value to just before the memory value instead of
moving memory value upward.

Convert Pre-Index Load Pattern to the Canonical Form:

    address = Add(base, offset)                    address = Nop
    ...                                            ...
    ...                                            newAddress = Add(base, offset)
    memory = Load(base, offset)            -->     memory = Load(base, offset)
    ...                                            ...
    parent = B3Opcode(address, ...)                parent = B3Opcode(newAddress, ...)

Convert Pre-Index Store Pattern to the Canonical Form:

    address = Add(base, offset)                    address = Nop
    ...                                            ...
    ...                                            newAddress = Add(base, offset)
    memory = Store(value1, base, offset)   -->     memory = Store(value1, base, offset)
    ...                                            ...
    parent = B3Opcode(address, ...)                parent = B3Opcode(newAddress, ...)

To move the address value downward, we need to make sure that no use reference of address between
the address and memory values.

* b3/B3CanonicalizePrePostIncrements.cpp:
(JSC::B3::canonicalizePrePostIncrements):
* b3/B3Generate.cpp:
(JSC::B3::generateToAir):
* b3/B3ValueKey.h:
* b3/B3ValueKeyInlines.h:
(JSC::B3::ValueKey::ValueKey):
* b3/testb3.h:
* b3/testb3_3.cpp:
(testLoadWithStorePreIndex32):
(testStorePreIndex32):
(testStorePreIndex64):
(testStorePostIndex32):
(testStorePostIndex64):
(addShrTests):
* runtime/OptionsList.h:


Canonical link: https://commits.webkit.org/240951@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@281587 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,77 @@
+2021-08-25  Yijia Huang  <yijia_huang@apple.com>
+
+        [ARM64] Fix pre-index address mode
+        https://bugs.webkit.org/show_bug.cgi?id=229175
+
+        Reviewed by Saam Barati.
+
+        This patch fixes the canonicalization phase for pre/post-increment address mode
+        due to the potential bugs commented on in the previous patch
+        https://bugs.webkit.org/show_bug.cgi?id=228538. And this patch removed the 
+        temporary fix in https://bugs.webkit.org/show_bug.cgi?id=229211.
+
+        Previously, the pre-index address mode for Load instruction convert the pattern 
+        to the canonical form like this:
+
+            address = Add(base, offset)       address = Add(base, offset)
+            ...                          -->  newMemory = Load(base, offset)
+            ...                               ...
+            memory = Load(base, offset)       memory = Identity(newMemory)
+
+        which is wrong. Assume "..." contains a store to a memory location that aliases for address:
+
+            address = Add(base, offset)       address = Add(base, offset)
+            ...                          -->  newMemory = Load(base, offset)
+            ...                               ...
+            Store(value1, address)            Store(value1, address)
+            memory = Load(base, offset)       memory = Identity(newMemory)
+
+        The loaded value should always be value1 which is not true after the conversion.
+        So, moving the load above the store is semantically incorrect because it's not identical to
+        the behavior of the original program. In this case, maybe we should apply alias analysis to
+        detect the violations of reference updating.
+
+        To solve this problem, we moves the address value to just before the memory value instead of
+        moving memory value upward.
+
+        Convert Pre-Index Load Pattern to the Canonical Form:
+
+            address = Add(base, offset)                    address = Nop
+            ...                                            ...
+            ...                                            newAddress = Add(base, offset)
+            memory = Load(base, offset)            -->     memory = Load(base, offset)
+            ...                                            ...
+            parent = B3Opcode(address, ...)                parent = B3Opcode(newAddress, ...)
+
+        Convert Pre-Index Store Pattern to the Canonical Form:
+
+            address = Add(base, offset)                    address = Nop
+            ...                                            ...
+            ...                                            newAddress = Add(base, offset)
+            memory = Store(value1, base, offset)   -->     memory = Store(value1, base, offset)
+            ...                                            ...
+            parent = B3Opcode(address, ...)                parent = B3Opcode(newAddress, ...)
+
+        To move the address value downward, we need to make sure that no use reference of address between
+        the address and memory values.
+
+        * b3/B3CanonicalizePrePostIncrements.cpp:
+        (JSC::B3::canonicalizePrePostIncrements):
+        * b3/B3Generate.cpp:
+        (JSC::B3::generateToAir):
+        * b3/B3ValueKey.h:
+        * b3/B3ValueKeyInlines.h:
+        (JSC::B3::ValueKey::ValueKey):
+        * b3/testb3.h:
+        * b3/testb3_3.cpp:
+        (testLoadWithStorePreIndex32):
+        (testStorePreIndex32):
+        (testStorePreIndex64):
+        (testStorePostIndex32):
+        (testStorePostIndex64):
+        (addShrTests):
+        * runtime/OptionsList.h:
+
 2021-08-25  Xan Lopez  <xan@igalia.com>
 
         [JSC] Infinite loop in for...in after r280760

Source/JavaScriptCore/b3/B3Generate.cpp

@@ -118,7 +118,7 @@ void generateToAir(Procedure& procedure)
     lowerMacrosAfterOptimizations(procedure);
     legalizeMemoryOffsets(procedure);
     moveConstants(procedure);
-    if (Options::useB3CanonicalizePrePostIncrements() && procedure.optLevel() >= 2)
+    if (procedure.optLevel() >= 2)
         canonicalizePrePostIncrements(procedure);
     eliminateDeadCode(procedure);
 

Source/JavaScriptCore/b3/B3ValueKey.h

@@ -56,7 +56,7 @@ class ValueKey {
     {
     }
 
-    ValueKey(Value* child, int64_t value);
+    ValueKey(Value* child, int32_t offset);
 
     ValueKey(Kind, Type, Value* child);
 

Source/JavaScriptCore/b3/B3ValueKeyInlines.h

@@ -33,12 +33,14 @@
 
 namespace JSC { namespace B3 {
 
-inline ValueKey::ValueKey(Value* child, int64_t value)
+inline ValueKey::ValueKey(Value* child, int32_t offset)
 {
     m_kind = Oops;
     m_type = Void;
-    u.indices[0] = child->index();
-    u.value = value;
+    // The observation is that when both child->index() and offset are 0's,
+    // HashMap would not accept the ValueKey.
+    u.indices[0] = child->index() + 1;
+    u.indices[1] = offset + 1;
 }
 
 inline ValueKey::ValueKey(Kind kind, Type type, Value* child)

Source/JavaScriptCore/b3/testb3.h

@@ -1164,6 +1164,7 @@ void addCopyTests(const char* filter, Deque<RefPtr<SharedTask<void()>>>&);
 bool shouldRun(const char* filter, const char* testName);
 
 void testLoadPreIndex32();
+void testLoadWithStorePreIndex32();
 void testLoadPreIndex64();
 void testLoadPostIndex32();
 void testLoadPostIndex64();

Source/JavaScriptCore/b3/testb3_3.cpp

@@ -102,6 +102,83 @@ void testLoadPreIndex32()
     CHECK_EQ(invoke<int32_t>(*code, bitwise_cast<intptr_t>(ptr)), test());
 }
 
+void testLoadWithStorePreIndex32()
+{
+    if (Options::defaultB3OptLevel() < 2)
+        return;
+
+    int32_t nums[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
+    int32_t* ptr = nums;
+
+    Procedure proc;
+    BasicBlock* root = proc.addBlock();
+    BasicBlock* loopTest = proc.addBlock();
+    BasicBlock* loopBody = proc.addBlock();
+    BasicBlock* done = proc.addBlock();
+
+    Variable* r = proc.addVariable(Int32);
+    Variable* p = proc.addVariable(Int64);
+
+    // ---------------------- Root_Block
+    // r1 = 0
+    // Upsilon(r1, ^r2)
+    // p1 = addr
+    // Upsilon(p1, ^p2)
+    Value* r1 = root->appendIntConstant(proc, Origin(), Int32, 0);
+    root->appendNew<VariableValue>(proc, B3::Set, Origin(), r, r1);
+    Value* p1 = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0);
+    root->appendNew<VariableValue>(proc, B3::Set, Origin(), p, p1);
+    root->appendNewControlValue(proc, Jump, Origin(), FrequentedBlock(loopTest));
+
+    // ---------------------- Loop_Test_Block
+    // loop:
+    // p2 = Phi()
+    // r2 = Phi()
+    // if r2 >= 10 goto done
+    Value* r2 = loopTest->appendNew<VariableValue>(proc, B3::Get, Origin(), r);
+    Value* p2 = loopTest->appendNew<VariableValue>(proc, B3::Get, Origin(), p);
+    Value* cond = loopTest->appendNew<Value>(proc, AboveEqual, Origin(), r2, loopTest->appendNew<Const32Value>(proc, Origin(), 10));
+    loopTest->appendNewControlValue(proc, Branch, Origin(), cond, FrequentedBlock(done), FrequentedBlock(loopBody));
+
+    // ---------------------- Loop_Body_Block
+    // p3 = p2 + 1
+    // Upsilon(p3, ^p2)
+    // p3' = &p3
+    // store(5, p3')
+    // r3 = r2 + load(p3)
+    // Upsilon(r3, ^r2)
+    // goto loop
+    Value* p3 = loopBody->appendNew<Value>(proc, Add, Origin(), p2, loopBody->appendNew<Const64Value>(proc, Origin(), 4));
+    loopBody->appendNew<VariableValue>(proc, B3::Set, Origin(), p, p3);
+    Value* p3Prime = loopBody->appendNew<Value>(proc, Opaque, Origin(), p3);
+    loopBody->appendNew<MemoryValue>(proc, Store, Origin(), loopBody->appendNew<Const32Value>(proc, Origin(), 5), p3Prime);
+    Value* r3 = loopBody->appendNew<Value>(proc, Add, Origin(), r2, loopBody->appendNew<MemoryValue>(proc, Load, Int32, Origin(), p3));
+    loopBody->appendNew<VariableValue>(proc, B3::Set, Origin(), r, r3);
+    loopBody->appendNewControlValue(proc, Jump, Origin(), FrequentedBlock(loopTest));
+
+    // ---------------------- Done_Block
+    // done:
+    // return r2
+    done->appendNewControlValue(proc, Return, Origin(), r2);
+
+    proc.resetReachability();
+    validate(proc);
+    fixSSA(proc);
+
+    auto code = compileProc(proc);
+
+    auto test = [&] () -> int32_t {
+        int32_t r = 0;
+        while (r < 10) {
+            *++ptr = 5;
+            r += *ptr;
+        }
+        return r;
+    };
+
+    CHECK_EQ(invoke<int32_t>(*code, bitwise_cast<intptr_t>(ptr)), test());
+}
+
 void testLoadPreIndex64()
 {
     if (Options::defaultB3OptLevel() < 2)
@@ -344,6 +421,7 @@ void testStorePreIndex32()
     auto code = compileProc(proc);
     if (isARM64())
         checkUsesInstruction(*code, "#4]!");
+
     intptr_t res = invoke<intptr_t>(*code, bitwise_cast<intptr_t>(ptr), 4);
     ptr = bitwise_cast<int32_t*>(res);
     CHECK_EQ(nums[2], *ptr);
@@ -368,6 +446,9 @@ void testStorePreIndex64()
     root->appendNewControlValue(proc, Return, Origin(), preIncrement);
 
     auto code = compileProc(proc);
+    if (isARM64())
+        checkUsesInstruction(*code, "#8]!");
+
     intptr_t res = invoke<intptr_t>(*code, bitwise_cast<intptr_t>(ptr), 4);
     ptr = bitwise_cast<int64_t*>(res);
     CHECK_EQ(nums[2], *ptr);
@@ -396,6 +477,7 @@ void testStorePostIndex32()
     auto code = compileProc(proc);
     if (isARM64())
         checkUsesInstruction(*code, "], #4");
+
     intptr_t res = invoke<intptr_t>(*code, bitwise_cast<intptr_t>(ptr), 4);
     ptr = bitwise_cast<int32_t*>(res);
     CHECK_EQ(nums[1], 4);
@@ -423,6 +505,7 @@ void testStorePostIndex64()
     auto code = compileProc(proc);
     if (isARM64())
         checkUsesInstruction(*code, "], #8");
+
     intptr_t res = invoke<intptr_t>(*code, bitwise_cast<intptr_t>(ptr), 4);
     ptr = bitwise_cast<int64_t*>(res);
     CHECK_EQ(nums[1], 4);
@@ -4097,17 +4180,15 @@ void addShrTests(const char* filter, Deque<RefPtr<SharedTask<void()>>>& tasks)
     RUN(testZShrArgImm32(0xffffffff, 1));
     RUN(testZShrArgImm32(0xffffffff, 63));
 
-    if (Options::useB3CanonicalizePrePostIncrements()) {
-        RUN(testLoadPreIndex32());
-        RUN(testLoadPreIndex64());
-        RUN(testLoadPostIndex32());
-        RUN(testLoadPostIndex64());
+    RUN(testLoadPreIndex32());
+    RUN(testLoadPreIndex64());
+    RUN(testLoadPostIndex32());
+    RUN(testLoadPostIndex64());
 
-        RUN(testStorePreIndex32());
-        RUN(testStorePreIndex64());
-        RUN(testStorePostIndex32());
-        RUN(testStorePostIndex64());
-    }
+    RUN(testStorePreIndex32());
+    RUN(testStorePreIndex64());
+    RUN(testStorePostIndex32());
+    RUN(testStorePostIndex64());
 }
 
 #endif // ENABLE(B3_JIT)

Source/JavaScriptCore/runtime/OptionsList.h

@@ -437,7 +437,6 @@ bool canUseWebAssemblyFastMemory();
     v(Unsigned, maxB3TailDupBlockSize, 3, Normal, nullptr) \
     v(Unsigned, maxB3TailDupBlockSuccessors, 3, Normal, nullptr) \
     v(Bool, useB3HoistLoopInvariantValues, false, Normal, nullptr) \
-    v(Bool, useB3CanonicalizePrePostIncrements, false, Normal, nullptr) \
     \
     v(Bool, useDollarVM, false, Restricted, "installs the $vm debugging tool in global objects") \
     v(OptionString, functionOverrides, nullptr, Restricted, "file with debugging overrides for function bodies") \