[webkit-changes] [WebKit/WebKit] f29853: [JSC] Fix B3CanonicalizePrePostIncrements with con...

[Yijia Huang]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: f29853f4b11b60c3257328ebc1ebe2b4bd7d2f9d
      https://github.com/WebKit/WebKit/commit/f29853f4b11b60c3257328ebc1ebe2b4bd7d2f9d
  Author: Yijia Huang <yijia_huang at apple.com>
  Date:   2024-08-09 (Fri, 09 Aug 2024)

  Changed paths:
    M Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp
    M Source/JavaScriptCore/b3/B3Generate.cpp
    M Source/JavaScriptCore/b3/B3LowerToAir.cpp
    M Source/JavaScriptCore/b3/B3LowerToAir32_64.cpp
    M Source/JavaScriptCore/b3/testb3.h
    M Source/JavaScriptCore/b3/testb3_3.cpp

  Log Message:
  -----------
  [JSC] Fix B3CanonicalizePrePostIncrements with control and alias analysis
https://bugs.webkit.org/show_bug.cgi?id=277807
rdar://133468869

Reviewed by Yusuke Suzuki.

In B3ReduceStrength, the Load and Store nodes have reductions:

              Turn This                                     Into This
--------------------------------------------------------------------------------------------
address = Add(base, offset1)              |  address = Add(base, offset1)
memory  = Load(address, offset2)          |  memory  = Load(base, offset1 + offset2)
--------------------------------------------------------------------------------------------
address = Add(base, offset1)              |  address = Add(base, offset1)
memory  = Store(value, address, offset2)  |  memory  = Store(value, base, offset1 + offset2)

In B3LowerToAir, we have the peephole optimizations for PrePostIndex forms:

        Turn Canonical Form                            Into PrePostIndex Form
-------------------------------------------------------------------------------------------------
address = Add(base, offset)               |  Move %base %address
memory  = Load(base, offset)              |  MoveWithIncrement (%address, prefix(offset)) %memory
-------------------------------------------------------------------------------------------------
address = Add(base, offset)               |  Move %base %address
memory  = Load(base, 0)                   |  MoveWithIncrement (%address, postfix(offset)) %memory
-------------------------------------------------------------------------------------------------
address = Add(base, Offset)               |  Move %base %address
memory  = Store(value, base, Offset)      |  MoveWithIncrement %value (%address, prefix(offset))
-------------------------------------------------------------------------------------------------
address = Add(base, Offset)               |  Move %base %address
memory  = Store(value, base, 0)           |  MoveWithIncrement %value (%address, postfix(offset))

The previous implementation of B3CanonicalizePrePostIncrements, converting PrePostIndex candidate
to canonical form, is wrong. This is because we cannot move the Load node directly to just after
the Add node without alias analysis to detect the violations of reference update. For example:

            Turn This                              Into This
---------------------------------------------------------------------------
    address = Add(base, offset)      |    address = Add(base, offset)
    ...                              |    newMemory = Load(base, offset)
    ...                              |    ...
    Store(value, address)            |    Store(value, address)
    memory = Load(base, offset)      |    memory = Identity(newMemory)

That conversion is semantically incorrect since the Store node can update the value in the address.
To fix this issue, this patch transforms all possible PrePostIndex candidates to these canonical
forms with control and alias analysis:

                                  Turn Candidate                       Into Canonical Form
--------------------------------------------------------------------------------------------------
                      |  address = Add(base, offset)         |  address = Nop
                      |  ...                                 |  ...
 PreIndex Load/Store  |  ...                                 |  newAddress = Add(base, offset)
      Pattern         |  memory = MemoryValue(base, offset)  |  memory = MemoryValue(base, offset)
                      |  ...                                 |  ...
                      |  use = B3Opcode(address, ...)        |  use = B3Opcode(newAddress, ...)
--------------------------------------------------------------------------------------------------
                      |  ...                                 |  newOffset = Constant
                      |  ...                                 |  newAddress = Add(base, newOffset)
 PostIndex Load/Store |  memory = MemoryValue(base, 0)       |  memory = MemoryValue(base, 0)
       Pattern        |  ...                                 |  ...
                      |  address = Add(base, offset)         |  address = Identity(newAddress)

PreIndex Load/Store Transform
When is it OK to move the address to the newAddress that is just right before the memory?
1. The basic blocks of the address and the memory must be control equivalent.
2. The basic block of the memory dominate all uses of the address.

PostIndex Load/Store Transform
When is it OK to move the address to the newAddress that is just right before the memory?
1. The basic blocks of the memory and the address must be control equivalent.

With those conditions satisfied, the code motion should be safe without changing the program's behavior.
Since this phase does not provide significant benefits on benchmarks, we will leave it disabled. However,
this patch does resolve the previous issue in B3CanonicalizePrePostIncrements.

* Source/JavaScriptCore/b3/B3CanonicalizePrePostIncrements.cpp:
(JSC::B3::canonicalizePrePostIncrements):
* Source/JavaScriptCore/b3/B3Generate.cpp:
(JSC::B3::generateToAir):
* Source/JavaScriptCore/b3/B3LowerToAir.cpp:
* Source/JavaScriptCore/b3/B3LowerToAir32_64.cpp:
* Source/JavaScriptCore/b3/testb3.h:
* Source/JavaScriptCore/b3/testb3_3.cpp:
(testLoadPreIndex32):
(testLoadPreIndex64):
(testLoadPostIndex32):
(testLoadPostIndex64):
(testLoadPreIndex32WithStore):
(testStorePreIndex64):
(addShrTests):
* Source/JavaScriptCore/runtime/OptionsList.h:

Canonical link: https://commits.webkit.org/282052@main



To unsubscribe from these emails, change your notification settings at https://github.com/WebKit/WebKit/settings/notifications