SILOptimizer: don't remove empty conflicting access scopes

[eeckstein]

Empty access scopes can be a result of e.g. redundant-load-elimination. It is important to keep such access scopes (if they might be conflicting) to detect access violations at runtime.
This PR removes the simple unconditional dead access scope elimination optimizations from instruction simplification and dead-code elimination.

To compensate for that, I added new DeadAccessScopeElimination pass. For example, it removes:

  %2 = begin_access [modify] [dynamic] %1
  ...                                       // no uses of %2
  end_access %2

However, dead conflicting access scopes are not removed.
If a conflicting scope becomes dead because an optimization e.g. removed a load, it is still important to get an access violation at runtime.
Even a propagated value of a redundant load from a conflicting scope is undefined.

  %2 = begin_access [modify] [dynamic] %1
  store %x to %2
  %3 = begin_access [read] [dynamic] %1    // conflicting with %2!
  %y = load %3
  end_access %3
  end_access %2
  use(%y)

After redundant-load-elimination:

  %2 = begin_access [modify] [dynamic] %1
  store %x to %2
  %3 = begin_access [read] [dynamic] %1    // now dead, but still conflicting with %2
  end_access %3
  end_access %2
  use(%x)                                  // propagated from the store, but undefined here!

In this case the scope %3 is not removed because it's important to get an access violation error at runtime before the undefined value %x is used.

This pass considers potential conflicting access scopes in called functions.
But it does not consider potential conflicting access in callers (because it can't!).
However, optimizations, like redundant-load-elimination, can only do such transformations if the outer access scope is within the function, e.g.

bb0(%0 : $*T):     // an inout from a conflicting scope in the caller
  store %x to %0
  %3 = begin_access [read] [dynamic] %1
  %y = load %3     // cannot be propagated because it cannot be proved that %1 is the same address as %0
  end_access %3

All those checks are only done for dynamic access scopes, because they matter for runtime exclusivity checking.
Dead static scopes are removed unconditionally.

rdar://164571252

[eeckstein]

@swift-ci smoke test

[eeckstein]

@swift-ci apple silicon benchmark

[eeckstein]

@swift-ci smoke test windows

[atrick]

Dead code elimination should definitely be able to delete empty accesses.

From the test case, without seeing the SIL, it looks like an RLE bug. RLE can't reuse a load from a conflicting access scope without creating a dependency on the begin_access.

[eeckstein]

From the test case, without seeing the SIL, it looks like an RLE bug. RLE can't reuse a load from a conflicting access scope without creating a dependency on the begin_access.

For RLE this would work. However I found that for dead-store elimination this would not work, because it would need to consider potential conflicts in called functions, i.e. it's not a function-local problem.

Therefore I decided to go another way. Instead of adding logic to RLE and DSE (and potentially other optimizations) I removed the simple unconditional dead-access-scope eliminations and added a new pass which can remove dead scopes - by considering conflicts.

[eeckstein]

@swift-ci test

[eeckstein]

@swift-ci apple silicon benchmark

[eeckstein]

@swift-ci test

[eeckstein]

@swift-ci apple silicon benchmark

[eeckstein]

@swift-ci test

[eeckstein]

@swift-ci apple silicon benchmark

[atrick]

For RLE this would work. However I found that for dead-store elimination this would not work, because it would need to consider potential conflicts in called functions, i.e. it's not a function-local problem.

I'm not sure why removing a dead store would require a conflicting access to be preserved. If the stored value is never used, then there's no real conflict. It's good for optimization to eliminate conflicting accesses if the result of the conflicting access is not observable.

[eeckstein]

I'm not sure why removing a dead store would require a conflicting access to be preserved.

This can happen if the dead store is in an enclosing scope of a conflicting inner scope. Even with the "fix" in RLE, this would be the case in the tests.test("copyable type") test I added.

[atrick]

I'm not sure why removing a dead store would require a conflicting access to be preserved.

This can happen if the dead store is in an enclosing scope of a conflicting inner scope. Even with the "fix" in RLE, this would be the case in the tests.test("copyable type") test I added.

For the record, until now, we always expected that the optimizer would remove such truly dead access scopes, regardless of whether they conflict with another access. I don't have an example on hand that shows how important it is to remove dead access scopes, but I know runtime exclusivity checks are very often the performance bottleneck as we get better at avoiding retain/release. The exclusivity runtime traps are also very difficult to analyze in release builds; we'd rather not trigger them at -O unless there is a real observable conflict (that could cause the program to misbehave).

Consider load elimination. Dead loads can simply be removed along with their access scope. Dead stores, like dead loads, can simply be removed along with their access scope because the stored value is never observered.

Redundant loads are different because the program observes the value as if the memory were accessed at the point of the original load. So, removing a redundant load creates a logical dependency on its access scope. We really should be representing that dependency in SIL.

I agree that removing dead access scopes means we need to be careful in any optimization that removes redundant loads, like mem2reg, RLE, and maybe others. So that introduces some burden on the optimizer. But, again, the dependency on the access scope is real, so it's cheating to remove the load without a mark_dependence.

In the "copyable" test case, this line semantically copies the value in C2.s, so there is no observable conflict:

let other = c.getS()

[eeckstein]

So, removing a redundant load creates a logical dependency on its access scope. We really should be representing that dependency in SIL.

Originally I wanted to do that. However that would mean that many followup optimizations need to correctly maintain this mark_dependence. This would create endless complexity in the optimizer. For example, If the value of a removed load is a stored constant, the constant propagation pass would need to "move" the mark_dependence to the instruction where the constant is propagated to. Even worse, bugs in such dependence updating logic wouldn't be found (until someone notices a missing runtime error for wrong code by chance).

Compared to that, DeadAccessScopeElimination is a simple and robust solution which removes the burden of many optimization passes to handle access scope dependencies.

In the "copyable" test case, this line semantically copies the value in C2.s, so there is no observable conflict:

Forgot to mention that the problem with DCE also shows up in the non-copyable test case.

[atrick]

Compared to that, DeadAccessScopeElimination is a simple and robust solution which removes the burden of many optimization passes to handle access scope dependencies.

I have no problem with a separate pass. I just want to make it clear that it's legal to remove dead conflicting scopes but we're choosing not to do that (yet) to avoid representing the dependency created by redundant loads.

[eeckstein]

I just want to make it clear that it's legal to remove dead conflicting scopes but we're choosing not to do that (yet) to avoid representing the dependency created by redundant loads.

It's not that simple. We have a similar problem with dead store elimination (and potentially other optimizations).
This code from the test case,

  mutating func add(_ other: borrowing Self) {
    i += other.i
    i += other.i
    print(self.i, other.i)
  }
...
  func foo() {
    nc.add(nc)
  }
...
  C1().foo()

after inlining everything and de-virtualizing C1's release, is translated to (simplified):

  %1 = alloc_ref $C1
  %2 = ref_element_addr %1, #nc
  store %initValueOfNC to %2    // initialize nc in `C1`'s initializer
  %3 = begin_access [dynamic] [modify] %2  // scope for mutating self argument of `add`
  %4 = begin_access [dynamic] [read] %2   // scope for reading `nc` for the argument to `add`: conflicting!
  %5 = load %4                   // read the `nc` argument - not dead
  end_access %4
  store %updatedValue to %3         // update `nc.i` in `add` - dead, because of the following `dealloc_ref`
  apply %print(%updatedValue)
  end_access %3
  dealloc_ref %1

Let's assume we even don't run RLE, so we don't remove the redundant load.
When DCE removes the dead store, the outer scope %3 becomes dead. If we remove this dead scope there wouldn't be a runtime error and we would print the "wrong" (= undefined) value.

[atrick]

after inlining everything and de-virtualizing C1's release, is translated to (simplified):

  %1 = alloc_ref $C1
  %2 = ref_element_addr %1, #nc
  store %initValueOfNC to %2    // initialize nc in `C1`'s initializer
  %3 = begin_access [dynamic] [modify] %2  // scope for mutating self argument of `add`
  %4 = begin_access [dynamic] [read] %2   // scope for reading `nc` for the argument to `add`: conflicting!
  %5 = load %4                   // read the `nc` argument - not dead
  end_access %4
  store %updatedValue to %3         // update `nc.i` in `add` - dead, because of the following `dealloc_ref`
  apply %print(%updatedValue)
  end_access %3
  dealloc_ref %1

Thanks for that example. This a different kind of problem. I was only considering the transformation that creates a dead access scope. But here, the invalid optimization has already happened before the access is dead, probably based on incorrect alias information. We need to preserve the conflicting access to catch that incorrect assumption about aliasing. So, I agree with your strategy. We want optimization to benefit from assuming that accesses cannot conflict. But since we can't easily represent all those assumptions, we need to enforce all access conflicts just in case.

[eeckstein]

But here, the invalid optimization has already happened before the access is dead, probably based on incorrect alias information.

What kind of invalid optimization do you mean? This is more or less the SIL after inlining and release-devirtualization. No other relevant optimization did run to produce this SIL.

[atrick]

What kind of invalid optimization do you mean? This is more or less the SIL after inlining and release-devirtualization. No other relevant optimization did run to produce this SIL.

The original code is

self.i += 
print(self.i, other.i)`

not

let x = self.i
print(x,other.i)

First someone must have removed the load of self.i. This might be another problem with RLE not creating a dependency.

The aliasing problem I was thinking of would be if the load of other.i was hoisted above the store to self.i. That's only legal if we assume dynamic access scopes cannot conflict.

At any rate, if the SIL you posted is correct out of SILGen, then it would be legal to delete the store and its access scope because the stored value is not observable.