[GSB] Improve handling of layout constraints #8253

swiftix · 2017-03-21T22:48:48Z

This PR addresses TODOs from #8241.

It supports merging for layout constraints, e.g., if both a _Trivial constraint and a _Trivial(64) constraint appear on a type parameter, we keep only _Trivial(64) as a more specific layout constraint. We do a similar thing for ref-counted/native-ref-counted. The overall idea is to keep the more specific of two compatible layout constraints.
The presence of a superclass constraint implies a layout constraint, e.g., a superclass constraint implies _Class or _NativeClass layout constraint.

swiftix · 2017-03-21T22:48:58Z

@swift-ci please smoke test

swiftix · 2017-03-21T22:49:15Z

@DougGregor Please have a look.

DougGregor · 2017-03-21T23:00:17Z

lib/AST/ASTContext.cpp

+    case LayoutConstraintKind::UnknownLayout:
+      return LayoutConstraint(
+          &LayoutConstraintInfo::UnknownLayoutConstraintInfo);
+    default:


We don't need the default, right? It'll end up suppressing warnings that we'd rather see when the switch isn't covering all of the cases.

DougGregor · 2017-03-21T23:02:14Z

lib/AST/LayoutConstraint.cpp

@@ -106,6 +110,24 @@ bool LayoutConstraintInfo::isNativeRefCountedObject(LayoutConstraintKind Kind) {
  return Kind == LayoutConstraintKind::NativeRefCountedObject;
 }

+bool LayoutConstraintInfo::isClass(LayoutConstraintKind Kind) {
+  return Kind == LayoutConstraintKind::Class;
+}


Should this return true when kind == LayoutConstraintKind::NativeClass? It "is" a class constraint, after all.

For this I introduced isAnyClass. This predicate is intended to be used to check the exact kind.

DougGregor · 2017-03-21T23:02:51Z

lib/AST/LayoutConstraint.cpp

+  }
+
+  if ((*this)->isClass() && Other->isNativeClass()) {
+    // Prefer a more concrete _NativeClass conttraint.


Missing a return Other; here

Oops. I forgot it while copy&pasting the code.

DougGregor · 2017-03-21T23:03:01Z

lib/AST/LayoutConstraint.cpp

+
+  if (Other->isClass() && (*this)->isNativeClass()) {
+    // Prefer a more concrete _NativeClass conttraint.
+  }


Missing a return *this here

DougGregor · 2017-03-21T23:03:50Z

lib/AST/LayoutConstraint.cpp

+    // Prefer a more concrete _NativeClass conttraint.
+  }
+
+  return LayoutConstraint::getUnknownLayout();


Might it be better to have a 2-D table to describe the results? It's pretty hard to make sure that this long sequence of if statements does the right thing.

@DougGregor Please have a look at the update that I pushed. It uses a 2-D table, but I'm not quite happy with it. The initialization of the table looked pretty ugly. I had to use macros to shorten it. And I'm still not quite convinced it is really easier. WDYT?

DougGregor · 2017-03-21T23:04:27Z

lib/AST/LayoutConstraint.cpp

+
+bool LayoutConstraintInfo::isRefCounted(LayoutConstraintKind Kind) {
+  return Kind == LayoutConstraintKind::RefCountedObject ||
+         Kind == LayoutConstraintKind::Class;


Shouldn't this also check for Kind == LayoutConstraintKind::NativeRefCountedObject and Kind == LayoutConstraintKind::NativeClass?

DougGregor · 2017-03-21T23:05:10Z

test/Generics/superclass_constraint.swift

@@ -100,6 +102,7 @@ class C2 : C, P4 { }
 // CHECK: superclassConformance3
 // CHECK: Requirements:
 // CHECK-NEXT: τ_0_0 : C2 [τ_0_0: Explicit @ {{.*}}:46]
+// CHECK-NEXT: τ_0_0 : _NativeClass [τ_0_0: Explicit @ {{.*}}:46 -> Superclass]


DougGregor · 2017-03-22T14:00:00Z

lib/AST/LayoutConstraint.cpp

+  if (!initializedMergeTable) {
+    // initialize the merge table once.
+    initializedMergeTable = true;
+    MERGE_TO(TrivialOfAtMostSize, Trivial, TrivialOfAtMostSize);


You should be able to use initializer list nesting { { ... }, { ... }, ... } to avoid the imperative initialization and initializedMergeTable bit. It'll also make sure that cases never get skipped, e.g., the missing merge(TrivialOfExactSize, TrivialOfExactSize) case.

DougGregor · 2017-03-22T14:05:56Z

lib/AST/LayoutConstraint.cpp

+    return LayoutConstraint::getUnknownLayout();
+
+  if ((*this)->getKind() == mergeKind)
+    return *this;


When the result is one of the sized cases (TrivialOfExactSize or TrivialOfAtMostSize), you'll need to do some checking of the size to determine the proper result. The lookup table doesn't always suffice in these cases... for example, the merge(TrivialOfExactSize, TrivialOfExactSize) case might produce TrivialOfExactSize (if the sizes are the same) and might produce TrivialOfAtMostSize or UnknownLayout (if the sizes are different; we can choose which semantics we prefer).

DougGregor · 2017-03-22T14:09:41Z

include/swift/AST/LayoutConstraint.h

@@ -170,6 +196,18 @@ class LayoutConstraintInfo : public llvm::FoldingSetNode {

  static bool isNativeRefCountedObject(LayoutConstraintKind Kind);

+  static bool isAnyRefCountedObject(LayoutConstraintKind Kind);


I don't see the reason for these static versions. They just seem to bloat the API.

DougGregor · 2017-03-22T14:11:03Z

include/swift/AST/LayoutConstraint.h

+    return isNativeClass(Kind);
+  }
+
+  bool isAnyClass() const {


I still find it confusing that we have isClass() and isAnyClass(). If someone wants the isClass() behavior, they can just check the kind exactly. Are you opposed to giving isClass() the behavior that isAnyClass() has (i.e., so isClass() answers the question "is it some kind of class?") and removing isAnyClass() entirely?

DougGregor · 2017-03-22T14:14:27Z

include/swift/AST/LayoutConstraint.h

+    return isAnyClass(Kind);
+  }
+
+  bool isRefCounted() const {


These behavior the way I'd expect: isRefCounted() is true for any layout kind that ensures reference-counting.

swiftix · 2017-03-22T17:35:42Z

@DougGregor I pushed the updated version. This one initializes all entries in the merge table. Does it look more reliable?

DougGregor · 2017-03-22T20:30:24Z

lib/AST/LayoutConstraint.cpp

+    // If we got here, it means that the Self == Other check above has failed.
+    // And that could only happen if either the kinds are different or
+    // size/alignment parameters are different.
+    return LayoutConstraint::getUnknownLayout();


Okay, it probably doesn't matter, but one could merge exact-size(N) and at-most-size(N) into exact-size(N).

- Add support for _Class and _NativeClass layout constraints, which are supposed to represent T: Superclass and P: class constraints in the future. - Use the re-factoring to also reduce the number of dynamic allocations when creating layout constraints. Simple non-parametrized layout constraints are now represented as statically allocated singletons (static members of LayoutConstraintInfo).

This PR addresses TODOs from swiftlang#8241. - It supports merging for layout constraints, e.g., if both a _Trivial constraint and a _Trivial(64) constraint appear on a type parameter, we keep only _Trivial(64) as a more specific layout constraint. We do a similar thing for ref-counted/native-ref-counted. The overall idea is to keep the more specific of two compatible layout constraints. - The presence of a superclass constraint implies a layout constraint, e.g., a superclass constraint implies _Class or _NativeClass

swiftix · 2017-03-22T23:51:00Z

@swift-ci please smoke test

DougGregor · 2017-03-23T03:40:14Z

Thank you!

DougGregor reviewed Mar 21, 2017

View reviewed changes

DougGregor reviewed Mar 22, 2017

View reviewed changes

swiftix added 2 commits March 22, 2017 16:37

swiftix force-pushed the wip-gsb-layout-constrains-fixes branch from 80d2ac7 to e1403c6 Compare March 22, 2017 23:50

swiftix merged commit e3bc23c into swiftlang:master Mar 23, 2017

		@@ -170,6 +196,18 @@ class LayoutConstraintInfo : public llvm::FoldingSetNode {

		static bool isNativeRefCountedObject(LayoutConstraintKind Kind);

		static bool isAnyRefCountedObject(LayoutConstraintKind Kind);

[GSB] Improve handling of layout constraints #8253

[GSB] Improve handling of layout constraints #8253

Uh oh!

Conversation

swiftix commented Mar 21, 2017

Uh oh!

swiftix commented Mar 21, 2017

Uh oh!

swiftix commented Mar 21, 2017

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!

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!

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!

swiftix commented Mar 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swiftix commented Mar 22, 2017

Uh oh!

DougGregor commented Mar 23, 2017

Uh oh!

Uh oh!