Fix speculative devirtualization to correctly use casted value

[slavapestov]

We used to emit this:

  checked_cast_br [exact] %0 : $Foo to $Bar, bb1, bb2

bb1(%1 : $Bar): // NOTE: %1 was not used anywhere
  %2 = unchecked_ref_cast %0 : $Foo to $Bar
  apply ...(..., %2)
  br ...

bb2:
  ...

This is not ownership SIL-safe, because we're re-using the original
operand, which will have already been consumed at that point.

The more immediate problem here is that this is actually not safe
when combined with other optimizations. Suppose that after the
speculative devirtualization, our function is inlined into another
function where the operand was an upcast. Now we have this code:

  %0 = upcast ...
  checked_cast_br [exact] %0 : $Foo to $Bar, bb1, bb2

bb1(%1 : $Bar):
  apply ...(..., %1) // NOTE: we're using the BB argument here
  br ...

bb2:
  ...

The SILCombiner will simplify the unchecked_ref_cast of the upcast
into an unchecked_ref_cast of the upcast's original operand. At
this point, we have an unchecked_ref_cast between two unrelated
class types. While this means the block bb1 is unreachable, we
might perform further optimizations on it before we run the cast
optimizer and delete it altogether.

In particular, the devirtualizer follows chains of reference cast
instructions, and it will get very confused if it finds this invalid
cast between unrelated types.

Fixes rdar://problem/57712094, https://bugs.swift.org/browse/SR-11916.

[slavapestov]

I wasn't able to come up with a reduced test case that triggers the original crash (the original bug report is while building SwiftNIO). The FileCheck tests for the output of speculative devirtualization are the best I could do.

[slavapestov]

@swift-ci Please smoke test

[slavapestov]

@swift-ci Please test source compatibility

[slavapestov]

@gottesmm You might be interested in this from the ownership SIL angle.

[atrick]

It's hard to follow the log comment for a couple reasons:
There's an extra %1 operand here:
%2 = unchecked_ref_cast %0 : $Foo to %1 : $Bar

I understand that checked_cast_br supports any conversion, while unchecked_ref_cast statically constrains the types, so I see how there could be a problem in theory. But it's not clear why the upcast would expose a problem--that should also have the same constraints. And I don't see any example of the problem scenario in the .sil test. What type was the upcast operating on and why couldn't that be unchecked_ref_cast'd?

[slavapestov]

@atrick The checked_cast_br is emitted by the devirtualizer in this case so its always a simple class downcast. The problem is rather that we're using the operand to the checked_cast_br instruction inside the success block.

The devirtualizer follows chains of cast instructions to find the 'original' type of the value. The problem is we had this:

%1 = unchecked_ref_cast (upcast %0 : $FirstSubclass to $BaseClass) to $SecondSubclass
class_method %1, SecondSubclass.someMethod

It was trying to devirtualize SecondSubclass.someMethod on an instance of FirstSubclass, which tripped an assertion.

The SIL tests were updated to CHECK for the pattern that is now generated, where instead of emitting an unchecked_ref_cast, we simply use the SILArgument in the success block of the checked_cast_br.

Does that clarify things?

[slavapestov]

@atrick Also I wasn't able to derive a stand-alone test case because in my small examples, the dead branches from the cast were deleted before we would do further optimization and devirtualization on them, so we never saw the invalid SIL. The SwiftNIO function that triggered this had something like 500 basic blocks.

However I think even as a cleanup in preparation for eventually converting speculative devirt to ownership SIL, this change makes sense to land in its current form. You can't use the original operand to a checked_cast_br in the success block, because it's been consumed.

[atrick]

@slavapestov Totally makes sense. I see how two bugs are being fixed at once. I run into the issue all the time that there's no way unit test certain mistakes in the optimizer.