Add doesCastPreserveOwnershipForType() to SIL/Utils/DynamicCasts.cpp #41952

atrick · 2022-03-22T17:12:24Z

Add doesCastPreserveOwnershipForType() to SIL/Utils/DynamicCasts.cpp

Added helpers in the AST Types.h

isNotPotentiallyAnyObject()
isPotentiallyBridgedValueOrExistentialType()

This formalizes logic for when cast operations forward
ownership. Various OSSA optimization rely on this for
correctness. This may fixes latent bugs throughout the optimizer.

I was compelled to fix this now because we want to make OSSA
optimizations across dynamic casts more aggressive. For example, we
want to optimize retain/release across enum formation.

atrick · 2022-03-22T17:13:08Z

This is the first step in fixing the OwnershipForwardingMixin. I'm not sure how to test this step yet.

atrick · 2022-03-23T06:32:36Z

@tbkka @jckarter I'm pretty comfortable with this logic now. Just need to add tests.

atrick · 2022-03-23T07:18:58Z

@swift-ci test

eeckstein

nice!

eeckstein · 2022-03-23T07:08:40Z

include/swift/AST/Types.h

@@ -1246,6 +1251,10 @@ class alignas(1 << TypeAlignInBits) TypeBase
  /// Whether this is the AnyObject type.
  bool isAnyObject();

+  /// Return true if this type will never be an AnyObject existential after
+  /// substitution.
+  bool isNotPotentiallyAnyObject();


nitpick: IMO it's better to avoid the negation in the function name -> isPotentiallyAnyObject.

eeckstein · 2022-03-23T07:13:58Z

lib/SIL/Utils/DynamicCasts.cpp

+// compatible cast cannot release any references within its operand's value
+// and cannot retain any references owned by its result.
+//
+// An type's ownership is preserved by a dynamic cast if it is both


"A type's ownership..."

eeckstein · 2022-03-23T07:19:41Z

lib/SIL/Utils/DynamicCasts.cpp

+    return true;
+  }
+  return doesCastPreserveOwnershipForType(getSourceFormalType())
+    && doesCastPreserveOwnershipForType(getTargetFormalType());


If I understand this code correctly, it's more conservative than what you described in the comment with (A), (B1) and (B2). Maybe you add a short comment for these conditions to relate them to A, B1, B2.

Specifically, does this include casting a class existential to a class which conforms to that protocol?
I'm asking because this is an important use case in libswift for fast casting (#41784).

From the comments:

// A type's ownership is preserved by a dynamic cast if it is neither // (A) a potentially bridged value // (isPotentiallyBridgedValueOrExistentialType()) // nor // (B) potentially wrapped in a transparent type // (isPotentiallyAnyObject())

A non-AnyObject class existential meets both requirements, as does a nominal class. I'll make sure there's some kind of test for that case.

@eeckstein I've lost track of the number of rewrites, but with the last attempt I think it's reaching the point where anyone can understand it. Still working on tests for all the interesting cases

atrick · 2022-03-24T06:54:57Z

@swift-ci test

atrick · 2022-03-25T06:14:43Z

@swift-ci test

atrick · 2022-03-25T06:14:52Z

@swift-ci test source compatibility

atrick · 2022-03-25T06:15:04Z

@swift-ci benchmark

atrick · 2022-04-03T18:42:03Z

@swift-ci test

atrick · 2022-04-03T18:42:10Z

@swift-ci benchmark

slavapestov · 2022-04-04T00:44:32Z

lib/AST/Type.cpp

+bool TypeBase::isPotentiallyAnyObject() {
+  Type unwrappedTy = lookThroughAllOptionalTypes();
+  if (auto archetype = unwrappedTy->getAs<ArchetypeType>()) {
+    return !archetype->hasRequirements();


hasRequirements() is a weird predicate. It doesn't check for all requirements, only conformance and superclass, and it's only used in one other place. Maybe you should just spell it out here so we can remove hasRequirements()?

Makes sense. Here:

bool TypeBase::isPotentiallyAnyObject() { Type unwrappedTy = lookThroughAllOptionalTypes(); if (auto archetype = unwrappedTy->getAs<ArchetypeType>()) { // Does archetype have any requirements that contradict AnyObject? // 'T : AnyObject' requires a layout constraint, not a conformance. return archetype->getConformsTo().empty() && !archetype->getSuperclass(); } return unwrappedTy->isAnyObject(); }

slavapestov · 2022-04-04T00:45:18Z

lib/AST/Type.cpp

+  if (auto archetype = unwrappedTy->getAs<ArchetypeType>()) {
+    return !archetype->hasRequirements();
+  }
+  return unwrappedTy->isAnyObject();


Do you want this to be true for Any as well as AnyObject?

AnyObject is very different from Any as regards casting behavior, so I suspect Andy wants this to only be true for AnyObject.

@slavapestov Good question. If we cared about Any, then the API would need to be called isAnyOrAnyObject(). I don't think it's possible for an Any to be AnyObject. That would imply that an existential contains another existential. I noticed the type system code does an Any-or-AnyObject checks in some places, but this API is specifically for potentially AnyObject.

The client of this API cares about cases like this. What's important to the client is that only AnyObject can hold a __SwiftValue. Any is never a __SwiftValue:

class C: Hashable, Equatable, P { static func == (lhs: C, rhs: C) -> Bool { return true } func hash(into hasher: inout Hasher) {} } let val: AnyHashable = C() let anyObject = val as AnyObject // metatype == "class __SwiftValue" let any = val as Any // metatype == "struct AnyHashable"

Added an AST helper in Types.h: - isPotentiallyAnyObject() This formalizes logic for when cast operations forward ownership. Various OSSA optimization rely on this for correctness. This fixes latent bugs throughout the optimizer. I was compelled to fix this now because we want to make OSSA optimizations across dynamic casts more aggressive. For example, we want to optimize retain/release across enum formation.

atrick · 2022-04-06T03:34:35Z

@swift-ci smoke test

atrick requested review from slavapestov, jckarter and eeckstein March 22, 2022 17:12

atrick force-pushed the add-cast-ownership branch 3 times, most recently from 408476e to 427fba1 Compare March 23, 2022 06:28

atrick changed the title ~~Add swift::isCastOwnershipPreservedForTypes()~~ Add doesCastPreserveOwnershipForType() to SIL/Utils/DynamicCasts.cpp Mar 23, 2022

atrick marked this pull request as ready for review March 23, 2022 06:31

atrick requested a review from tbkka March 23, 2022 06:31

eeckstein reviewed Mar 23, 2022

View reviewed changes

atrick force-pushed the add-cast-ownership branch 2 times, most recently from 0ce5011 to 690cd32 Compare March 24, 2022 06:54

atrick force-pushed the add-cast-ownership branch from 690cd32 to 51af7e9 Compare March 25, 2022 06:14

atrick force-pushed the add-cast-ownership branch from 51af7e9 to 52f3413 Compare April 1, 2022 16:16

slavapestov approved these changes Apr 4, 2022

View reviewed changes

slavapestov reviewed Apr 4, 2022

View reviewed changes

atrick added 3 commits April 5, 2022 09:10

Remove unused flag from canUseScalarCheckedCast

fd84606

[SIL Opaque] Always allow scalar casts

90ec837

atrick force-pushed the add-cast-ownership branch from 52f3413 to 90ec837 Compare April 6, 2022 03:28

atrick merged commit af11e34 into swiftlang:main Apr 6, 2022

atrick deleted the add-cast-ownership branch April 6, 2022 17:46

Add doesCastPreserveOwnershipForType() to SIL/Utils/DynamicCasts.cpp #41952

Add doesCastPreserveOwnershipForType() to SIL/Utils/DynamicCasts.cpp #41952

Uh oh!

Conversation

atrick commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atrick commented Mar 22, 2022

Uh oh!

atrick commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atrick commented Mar 23, 2022

Uh oh!

eeckstein left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atrick Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atrick commented Mar 24, 2022

Uh oh!

atrick commented Mar 25, 2022

Uh oh!

atrick commented Mar 25, 2022

Uh oh!

atrick commented Mar 25, 2022

Uh oh!

atrick commented Apr 3, 2022

Uh oh!

atrick commented Apr 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atrick commented Apr 6, 2022

Uh oh!

Uh oh!

atrick commented Mar 22, 2022 •

edited

Loading

atrick commented Mar 23, 2022 •

edited

Loading

atrick Mar 23, 2022 •

edited

Loading