Implement MultiPayloadEnum support for projectEnumValue #30635

tbkka · 2020-03-25T16:37:14Z

This code rearchitects and simplifies the projectEnumValue support by introducing a new TypeInfo subclass for each kind of enum, including trivial, no-payload, single-payload, and multi-payload enums. These classes are mostly private to the TypeLowering.cpp source file; the only change visible outside of that is a new base EnumTypeInfo that is a sibling of RecordTypeInfo. Three of these new classes are used for MultiPayload enum support:

"Unsupported" enums that we don't know how to project. This TI returns "don't know" answers for any request to project an enum value or an XI but otherwise has correct layout and case data.
"Simple" MP Enums that only use a separate tag value. This includes dynamically-laid-out generic enums, as well as enums whose payloads have no spare bits.
The general case of MP Enums that use spare bits, possibly in addition to a separate tag. This logic can only be used, of course, if we can in fact compute a spare bit mask that agrees with the compiler.

The final challenge is to choose one of the above three handlings for every MPE. In particular, there is still some work to robustly compute the spare bit mask for the third type above.

Resolves rdar://31666592

This code rearchitects and simplifies the projectEnumValue support by introducing a new `TypeInfo` subclass for each kind of enum, including trivial, no-payload, single-payload, and three different classes for multi-payload enums: * "UnsupportedEnum" that we don't understand. This will allow us to return correct "don't know" answers in cases where the runtime lacks enough information to accurately handle a particular enum. * MP Enums that only use a separate tag value. This includes generic enums and other dynamic layouts, as well as enums whose payloads have no spare bits. * MP Enums that use spare bits, possibly in addition to a separate tag. This logic can only be used, of course, if we can in fact compute a spare bit mask that agrees with the compiler. The final challenge is to choose one of the above three handlings for every MPE. The current code that computes the spare bit mask for the third type above has some flaws that need to be understood. Next step: Figure out spare bit mask computation. Any cases we can compute correctly based on the information available at runtime can be handled by one of the above three TI classes. For the remaining cases, we'll probably have to divert to "Unsupported" until we can arrange for the compiler to provide us with augmented data.

tbkka · 2020-03-25T16:45:57Z

This is "WIP" because there's still some work needed around the spare bit mask computation. The basic implementation I have here is sufficient to test everything on x86_64 and 32-bit platforms. In particular, there seems to be some question about whether visibility information that's only known to the compiler needs to be considered here.

There are a number of possible ways forward:

Query the memory reader to get target-specific data. This would be a simple way to extend the current code to support all targets. If that's sufficient, it would be an excellent answer.
Identify cases that cannot be handled correctly and divert them to UnsupportedEnumTypeInfo which fails all enum projection requests. That will allow the cases that do work to become available to consumers.
Teach the compiler to add more metadata for use by the runtime so that the runtime can produce spare bit mask information for all cases. Of course, I'd like to reduce the memory impact of this.

tbkka

I've peppered some expository comments throughout the code to help orient reviewers.

tbkka · 2020-03-25T16:47:48Z

include/swift/Reflection/ReflectionContext.h

    }
+    return EnumTI->projectEnumValue(getReader(), EnumAddress, CaseIndex);


@slavapestov Note that I've pushed all the real knowledge down out of the ReflectionContext and into new TypeInfo classes that are private to TypeLowering.cpp. I believe this is essentially the approach you were suggesting.

tbkka · 2020-03-25T16:50:52Z

include/swift/Reflection/TypeLowering.h

@@ -209,6 +215,65 @@ class RecordTypeInfo : public TypeInfo {
  }
 };

+/// Enums
+class EnumTypeInfo : public TypeInfo {


I decided it made the most sense for the new EnumTypeInfo to be a sibling of RecordTypeInfo, rather than a subclass. Enums have cases and need to be inspected to determine the active case, which makes them different than records.

tbkka · 2020-03-25T16:51:57Z

include/swift/SwiftRemoteMirror/SwiftRemoteMirror.h

@@ -231,6 +231,8 @@ int swift_reflection_projectExistential(SwiftReflectionContextRef ContextRef,
 ///
 /// Takes the address and typeref for an enum and determines the
 /// index of the currently-selected case within the enum.
+/// You can use this index with `swift_reflection_childOfTypeRef`
+/// to get detailed information about the specific case.


As @slavapestov pointed out, the getEnumCaseTypeRef wasn't needed, so it's been dropped.

tbkka · 2020-03-25T16:54:26Z

stdlib/public/Reflection/TypeLowering.cpp

+  return false;
+}
+
+class UnsupportedEnumTypeInfo: public EnumTypeInfo {


This is the first of several private subclasses of EnumTypeInfo that contain decoding knowledge for specific type of enum. I experimented with trying to unify enum decoding but that got unduly complex. Splitting out the different cases here dramatically simplified things.

tbkka · 2020-03-25T16:55:44Z

stdlib/public/Reflection/TypeLowering.cpp

+  }
+};
+
+class EmptyEnumTypeInfo: public EnumTypeInfo {


The Enum type info classes are broken more finely than just no/single/multi payload. Separating out "unsupported", "empty", and "trivial" enums into their own classes eliminated checks for those cases from other parts of the code.

tbkka · 2020-03-25T17:02:05Z

stdlib/public/Reflection/TypeLowering.cpp

+  template<typename IntegerType>
+  bool readMaskedInteger(remote::MemoryReader &reader,
+                         remote::RemoteAddress address,
+                         IntegerType *dest) const {


This uses a memory reader to gather bits from a payload into a single integer.

tbkka · 2020-03-25T17:02:43Z

stdlib/public/Reflection/TypeLowering.cpp

+
+// General multi-payload enum support for enums that do use spare
+// bits in the payload.
+class MultiPayloadEnumTypeInfo: public EnumTypeInfo {


MP enums that do not use spare bits can rely on the much simpler SimpleMultiPayloadEnumTypeInfo above.

stdlib/public/Reflection/TypeLowering.cpp

tbkka · 2020-03-25T17:05:52Z

stdlib/public/Reflection/TypeLowering.cpp

+        BitMask spareBitsMask(PayloadSize);
+        auto validSpareBitsMask = populateSpareBitsMask(Cases, spareBitsMask);
+
+        if (!validSpareBitsMask) {


Here's the critical logic that determines how to handle a particular MPE based on our current understanding.

tbkka · 2020-03-25T17:07:00Z

stdlib/public/Reflection/TypeLowering.cpp

@@ -1522,21 +2144,19 @@ class LowerType
    if (auto *ReferenceTI = dyn_cast<ReferenceTypeInfo>(TI))
      return TC.getReferenceTypeInfo(Kind, ReferenceTI->getReferenceCounting());

+    if (auto *EnumTI = dyn_cast<EnumTypeInfo>(TI)) {
+      if (EnumTI->isOptional() && Kind == ReferenceKind::Weak) {


Note the previous version of this code just used SubKind == SinglePayloadEnum as a test for something being an Optional. I've factored that out in order to make it easier to refine this decision.

jckarter · 2020-03-25T17:16:06Z

Rather than have the runtime try to recompute spare bit information from the payload types, it might be simpler to encode the spare bits used by a particular enum in its metadata, since we never use spare bits for runtime type layout and always know them at compile time. Since this also means that the spare bit mask would never vary among different generic instantiations of a generic enum, we could add the information to the enum's type context descriptor, which is shared among all generic instantiations and lives in __TEXT,__const, which should reduce the runtime memory impact of the added metadata.

tbkka · 2020-03-25T17:29:43Z

@jckarter I understood that generic MPEs never used the spare bit strategy, so that part already doesn't vary with different instantiations. @slavapestov also suggested adding spare bit info to the type metadata for fixed-layout MPEs that use it. I'm not sure if that would be simpler or not; my current code to walk the type tree and accumulate spare bit info is only a couple dozen lines of code, though I need to do some more tests to see if I missed any cases. My only real concern revolves around whether the compiler chooses MPE strategies based on information that's currently completely missing from the runtime metadata (such as visibility information).

jckarter · 2020-03-25T17:45:40Z

I'm pretty sure the layout in its full generality does rely on information that's not available to the runtime. Encoding the spare bit info in the enum metadata would also be more robust if we decide to rev the layout algorithm for newly-defined enums eventually too.

tbkka · 2020-03-25T18:38:55Z

"information that's not available to the runtime" -- Can you be more specific? I'd like to have a test case to verify that the RemoteMirror code fails on cases that it should not be able to handle.

stdlib/public/Reflection/TypeLowering.cpp

jckarter · 2020-03-25T23:45:52Z

"information that's not available to the runtime" -- Can you be more specific? I'd like to have a test case to verify that the RemoteMirror code fails on cases that it should not be able to handle.

To my best recollection, we get spare bits from the high byte of 64-bit object or function pointers, from the unused bits in enums, from padding between struct/tuple elements, and from the unused high bits of Builtin.IntNN fields when NN is not a power of two. Of those, I guess only the Builtin.IntNN case is strictly not reconstructable from runtime information. However, it seems like we would have to reimplement a lot of IRGen in order to be able to reconstruct the spare bit info for enum/struct padding.

…depended on it The experimental spare bits calculation helped to validate the structure of this code and the handling of various multi-payload enum details, but it cannot be done here in a way that will always agree with the compiler. So I've backed that out: The result can still handle certain multi-payload enums (those with dynamic layouts that can never utilize spare bits) but the remainder will require some additional work for the compiler to expose the real spare bit data for use in the runtime.

tbkka · 2020-03-26T20:10:50Z

@swift-ci Please test

swift-ci · 2020-03-26T20:13:20Z

Build failed
Swift Test Linux Platform
Git Sha - cfed291

tbkka · 2020-03-26T20:14:56Z

I believe I've resolved the spare bits question sufficient to make this mergeable in its current state. As such, I've removed the "WIP" from the PR title and plan to merge it as soon as it can pass tests.

tbkka · 2020-03-26T23:17:32Z

@swift-ci Please test

swift-ci · 2020-03-27T00:04:02Z

Build failed
Swift Test Linux Platform
Git Sha - 2ad9bec

tbkka · 2020-03-30T18:07:08Z

@swift-ci Please test Linux

swift-ci · 2020-03-30T18:58:12Z

Build failed
Swift Test Linux Platform
Git Sha - 2ad9bec

tbkka · 2020-03-30T22:07:27Z

@swift-ci Please test Linux

swift-ci · 2020-03-30T22:10:11Z

Build failed
Swift Test Linux Platform
Git Sha - 2ad9bec

tbkka · 2020-03-31T15:57:33Z

@swift-ci Please test

swift-ci · 2020-03-31T16:00:00Z

Build failed
Swift Test OS X Platform
Git Sha - 2ad9bec

swift-ci · 2020-03-31T16:00:19Z

Build failed
Swift Test Linux Platform
Git Sha - c42e15e

tbkka · 2020-03-31T18:44:03Z

@swift-ci Please smoke test Linux

tbkka · 2020-03-31T19:15:09Z

@swift-ci Please test Linux

tbkka · 2020-03-31T20:57:02Z

@swift-ci Please test Linux

swift-ci · 2020-03-31T22:28:11Z

Build failed
Swift Test Linux Platform
Git Sha - 481d09a

tbkka requested review from mikeash and slavapestov March 25, 2020 16:46

tbkka commented Mar 25, 2020

View reviewed changes

stdlib/public/Reflection/TypeLowering.cpp Show resolved Hide resolved

tbkka commented Mar 25, 2020

View reviewed changes

stdlib/public/Reflection/TypeLowering.cpp Outdated Show resolved Hide resolved

tbkka changed the title ~~WIP: Implement MultiPayloadEnum support for projectEnumValue~~ Implement MultiPayloadEnum support for projectEnumValue Mar 26, 2020

Merge branch 'master' into tbkka-remoteMirror-projectEnum

fcb87b7

Explicitly include MetadataValues.h

c42e15e

Avoid std::min with static const arguments

481d09a

tbkka merged commit 3c8fde7 into swiftlang:master Mar 31, 2020

tbkka deleted the tbkka-remoteMirror-projectEnum branch October 16, 2020 00:33

kastiglione mentioned this pull request Jan 16, 2021

[Reflection] Support OpaqueExistential in RecordTypeInfo::readExtraInhabitantIndex #35433

Merged

		}
		return EnumTI->projectEnumValue(getReader(), EnumAddress, CaseIndex);

Implement MultiPayloadEnum support for projectEnumValue #30635

Implement MultiPayloadEnum support for projectEnumValue #30635

Uh oh!

Conversation

tbkka commented Mar 25, 2020

Uh oh!

tbkka commented Mar 25, 2020

Uh oh!

tbkka 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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jckarter commented Mar 25, 2020

Uh oh!

tbkka commented Mar 25, 2020

Uh oh!

jckarter commented Mar 25, 2020

Uh oh!

tbkka commented Mar 25, 2020

Uh oh!

Uh oh!

Uh oh!

jckarter commented Mar 25, 2020

Uh oh!

tbkka commented Mar 26, 2020

Uh oh!

swift-ci commented Mar 26, 2020

Uh oh!

tbkka commented Mar 26, 2020

Uh oh!

tbkka commented Mar 26, 2020

Uh oh!

swift-ci commented Mar 27, 2020

Uh oh!

tbkka commented Mar 30, 2020

Uh oh!

swift-ci commented Mar 30, 2020

Uh oh!

tbkka commented Mar 30, 2020

Uh oh!

swift-ci commented Mar 30, 2020

Uh oh!

tbkka commented Mar 31, 2020

Uh oh!

swift-ci commented Mar 31, 2020

Uh oh!

swift-ci commented Mar 31, 2020

Uh oh!

tbkka commented Mar 31, 2020

Uh oh!

tbkka commented Mar 31, 2020

Uh oh!

tbkka commented Mar 31, 2020

Uh oh!

swift-ci commented Mar 31, 2020

Uh oh!

Uh oh!