SIL: fix the ownership computation of struct_extract and tuple_extract

[eeckstein]

A struct or tuple value can have "none" ownership even if its type is not trivial. This happens when the struct/tuple contains a non-trivial enum, but it's initialized with a trivial enum case (e.g. with Optional.none).

  %1 = enum $Optional<String>, #Optional.none!enumelt
  %2 = struct $S (%1)               // has ownership "none"
  %3 = struct_extract %2, #S.x       // should also have ownership "none" and not "guaranteed"

So far it got "guaranteed" ownership which is clearly wrong.

Fixes an assertion crash in redundant load elimination.

#80430
rdar://148311534

[eeckstein]

@swift-ci test

[eeckstein]

@swift-ci benchmark

[atrick]

Value ownership needs to be a function of an operation and its type, independent of its operands. I'm not aware that operand type affects ownership anywhere else (visitForwardInst is not called and should be deleted).

The problem is that value ownership determines the structural invariants of the SILValue. Like does it need a destroy_value, is it even allowed to have a destroy_value, or can it be passed to certain operands that have ownership qualifiers, like stores. So, if value ownership depends on its operand ownership, then we have problem: simplification of one OSSA value might propagate ownership changes in an unbounded way. That would require arbitrary amounts of SIL rewriting.

I don't understand exactly what the bug is here. Clearly the OSSA after load elimination is invalid. Maybe some utility or optimization is being "too smart" about whether a value is "dynamically trivial".

If we need to see that a nontrivial struct_extract has a dynamically trivial value, then that can be done by giving struct_extract an ownershipKind, just like all the other forwarding operations. Then, if some optimization wants to treat the struct_extract like a trivial, it can call setForwardingOwnershipKind(). But when that happens, it may need to update SIL for the new ownership kind. That should be striaghforward because, with this design, updating the SIL for ownership is only local--we only care about the value's immediate uses.

[atrick]

After considering, I agree that this approach is fine for projections like struct_extract and tuple_extract.

Adding an ownershipkind field is what we do elsewhere, but it is not a great approach because it is not serialized.

The reasoning is:

• A value with 'none' ownership can always be used where we expect owned or guaranteed. If we still have any assertions that don't allow 'none', we should really fix those. 'none' values can be copied or destroyed even though it is unnecessary.

• We are allowed to replace an owned/guaranteed value projection's operand with a 'none' value, but never the other way around.

Is it possible to assert when updating the operand of struct/tuple_extract that we never go from none to owned/guaranteed?

[eeckstein]

Is it possible to assert when updating the operand of struct/tuple_extract that we never go from none to owned/guaranteed?

Hm, we could add an assert in replace-all-uses

[meg-gupta]

LGTM.

@eeckstein
We may need this change for all CONSTANT_OR_NONE_OWNERSHIP_INST that are forwarding ? i.e value ownership is OwnershipKind::None when getForwardingOwnershipKind() is OwnershipKind::None ?

[eeckstein]

We may need this change for all CONSTANT_OR_NONE_OWNERSHIP_INST that are forwarding ?

I don't think we can run into this problem (with the trivial enum case) with any other of those forwarding instructions. For example, the operand of OpenExistentialValue should never have a none ownership