Introduce a simple request evaluator. #15917

DougGregor · 2018-04-13T05:55:38Z

Meant as a replacement for the barely-started iterative type checker,
introduce a simpler "evaluator" that can evaluate individual requests
(essentially, function objects with some additional API), caching
results as appropriate and detecting cycles. Start stubbing out what
is involved in replacing the iterative type checker with this infrastructure.

graydon

Bunch of nits but looks good!

graydon · 2018-04-13T06:24:21Z

include/swift/AST/AnyRequest.h

+///   - Hashing support (hash_value)
+///   - TypeID support (see swift/Basic/TypeID.h)
+///   - Cycle diagnostics operations:
+///


Did you forget 'OutputType breakCycle() const' here? It looks like it's present in a similar comment down below.

AnyRequest itself doesn't need it, because we only call the breakCycle operation on a request before it's been type-erased. We can add it here if we need it later.

graydon · 2018-04-13T06:24:45Z

include/swift/AST/Evaluator.h

+///
+///       void diagnoseCycle(DiagnosticEngine &diags) const;
+///       void noteCycleStep(DiagnosticEngine &diags) const;
+///       OutputType breakCycle() Const;


s/Const/const/

graydon · 2018-04-13T06:25:42Z

include/swift/AST/Evaluator.h

+///     static const bool isEverCached;
+///
+///       When false, the request's result will never be cached. Note that this
+///       also precludes cycle detection.  When true, the result will be


Why does not-caching preclude cycle detection?

Ah, it doesn't have to. I'm using the cache to detect the presence of a cycle, but there's no reason I have to do that... I have the stack of results to be satisfied and can unique there. Thanks!

graydon · 2018-04-13T06:27:37Z

include/swift/AST/Evaluator.h

+  /// request.
+  void diagnoseCycle(const AnyRequest &request);
+
+  /// Retrieve the result of produced by evaluating a request that can


s/result of/result/

graydon · 2018-04-13T06:28:27Z

include/swift/AST/Evaluator.h

+    return getResultUncached(request);
+  }
+
+  /// Retrieve the result of produced by evaluating a request that


s/result of/result/

graydon · 2018-04-13T06:28:38Z

include/swift/AST/Evaluator.h

+    return getResultUncached(request);
+  }
+
+  /// Produce the result of the request without caching.


s/result of/result/

Hey, this one is actually correct ;)

graydon · 2018-04-13T06:32:45Z

include/swift/AST/Evaluator.h

+  typename Request::OutputType getResultUncached(const Request &request) {
+    // Push this request onto the active-requests stack while we
+    // evaluate it.
+    activeRequests.push_back(request);


Why push/pop it if you can't / aren't going to detect cycles involving it?

I'm diagnosing cycles based on the stack, but you're right (per above) that I should use this for cycle detection as well.

graydon · 2018-04-13T06:33:37Z

include/swift/Basic/AnyValue.h

+
+namespace swift {
+
+/// Stores a value of any type satisfies a small set of requirements


s/any type satisfies/any type that satisfies/

graydon · 2018-04-13T06:40:30Z

include/swift/Basic/TypeID.h

+
+/// Form a type ID given a zone and type value.
+constexpr uint8_t formTypeID(uint8_t zone, uint8_t type) {
+  return (zone << 8) | type;


I'm probably doing my own arithmetic wrong, here but isn't a uint8_t shifted left by 8 bits always going to be 0? Maybe this should be returning a uint64_t?

This should absolutely be returning a uint64_t!

graydon · 2018-04-13T06:43:44Z

include/swift/Basic/TypeID.h

+                                                                        \
+public:                                                                 \
+  static const uint64_t value =                                         \
+    (templateID << (depth * 8)) | TypeID<Arg1>::value;                  \


Again here, I think the arithmetic is off, and you want to shift by (depth * 16). Assuming you mean to have [256 zones | 256 types] packed as a 16 bits in each of up-to-4 depths, in a 64-bit word.

Funny how it's the simple bit-manipulation math that I completely screwed up in this pull request. Thank you (and @huonw)

huonw · 2018-04-13T06:19:54Z

include/swift/Basic/AnyValue.h

+namespace swift {
+
+/// Stores a value of any type satisfies a small set of requirements
+/// (currenrly, just equatability).


currenrly -> currently.

huonw · 2018-04-13T06:22:15Z

include/swift/Basic/AnyValue.h

+
+  /// Cast to a specific (known) type.
+  template<typename T>
+  const T &castTo() const {


This could have an assert on the TypeId<T>::value.

huonw · 2018-04-13T06:27:11Z

include/swift/Basic/TypeID.h

+                                                                        \
+public:                                                                 \
+  static const uint64_t value =                                         \
+    (templateID << (depth * 8)) | TypeID<Arg1>::value;                  \


Shouldn't this be depth * 16. Also, could we could possibly avoid depth all together using (TypeID<Arg1>::value << 16) | templateID, assuming value is guaranteed to be non-zero (and/or static_assert that it's non-zero)?

Good idea, I've done this.

huonw · 2018-04-13T06:27:26Z

include/swift/Basic/TypeID.h

+
+/// Form a type ID given a zone and type value.
+constexpr uint8_t formTypeID(uint8_t zone, uint8_t type) {
+  return (zone << 8) | type;


Hm, this looks like overflow; should this return uint16_t? (and are casts needed to stop the intermediate overflowing?)

huonw · 2018-04-13T06:29:00Z

include/swift/Basic/TypeID.h

+SWIFT_TYPEID(C, float, 12);
+SWIFT_TYPEID(C, double, 13);
+SWIFT_TYPEID(C, bool, 14);
+  SWIFT_TYPEID(C, decltype(nullptr), 15);


Weird indentation?

huonw · 2018-04-13T06:29:25Z

include/swift/Sema/Requests.h

+
+struct TypeLoc;
+
+/// Request the type from the ith entry in the inheritance clause for the


huonw · 2018-04-13T06:30:34Z

include/swift/Sema/Requests.h

+  }
+};
+
+SWIFT_TYPEID(TypeChecker, InheritedTypeRequest, 1);


Doing these individually by hand seems prone to mistakes and problems with merge conflicts? Is it worth having (yet another) .def file?

Yeah, I will do something here. It's an annoying pile of preprocessor metaprogramming.

huonw · 2018-04-13T06:32:23Z

lib/Basic/AnyValue.cpp

+#include "swift/Basic/AnyValue.h"
+using namespace swift;
+
+AnyValue::HolderBase::~HolderBase() { }


This is the second definition of this: it's also above in lib/AST/Evaluator.cpp https://github.com/apple/swift/pull/15917/files#diff-dfb37be769b2d50ad4c150591bf84c59R22 ?

That one is AnyRequest::HolderBase::~HolderBase().

huonw · 2018-04-13T06:52:50Z

include/swift/AST/Evaluator.h

+///       When true, the request itself must provide an way to cache the
+///       results, e.g., in some external data structure. External caching
+///       should only be used when staging in the use of the evaluator into
+///       existing mutable data structures; new computations should not depend


Hm! Are we going to migrate to, say, every TypeLoc going through the Evaluator? If so, it kinda seems like we may want something with a more continuous memory layout? I'm thinking of basically doing an AoS-to-SoA transform, something like:

class Evaluator { // ... llvm::DenseMap<TypeId, AnyCache> caches; // ... } class AnyCache { // a type erased `llvm::DenseMap<Request, Request::OutputType>` }

or, even, going fully-static/killing our compile times:

template<typename Request...> class Evaluator { std::tuple<llvm::DenseMap<Request, Request::OutputType>...> caches; // or whatever the template magic to get a DenseMap for each request type is. // (plus extra magic to elide the map for externally cached requests.) } using SemaEvaluator = Evaluator< InheritedTypeRequest, SuperclassTypeRequest, ...>;

The getResultCached function would then first need to find the appropriate cache (a downside of the first approach is, of course, that this may be moderately expensive and so outweighs the benefits of that style vs. the [AnyRequest : AnyValue?] version), but I think the rest is the same.

This is a fantastic idea! Not the fully-static one, which would kill our layering and our compile times, but the type-erased AnyCache would be much better.

DougGregor · 2018-04-20T05:30:28Z

Some feedback addressed, but I haven't simplified TypeID definitions yet nor have I implemented @huonw 's AnyCache suggestion.

huonw · 2018-04-26T22:28:24Z

The double layer of X-macros for the zones is... slightly mind-bending, but makes sense.

rudkx · 2018-04-26T22:32:05Z

include/swift/Basic/DefineTypeIDZone.h

+//
+//    SWIFT_TYPEID_ZONE: The name of the Zone being defined, e.g.,
+//    SwiftAST for the Swift AST library's zone. All zones need to be defined
+//    a priori as 


....as....?

Fixed with the latest update, thanks

DougGregor · 2018-05-23T18:56:57Z

Several goodies in the latest update:

All requests (and all values) are printable, so we can inspect anything
Added SimpleRequest, which takes care of defining a new request with a simple payload (list of types) and automates basically all of it.
Online dependency tracking
Printing of the dependencies for a given request (as a tree), along with cached results, e.g.,

 `--InternallyCachedEvaluationRule(Binary: product) -> 2.461145e+02
     `--InternallyCachedEvaluationRule(Binary: sum) -> 5.859870e+00
     |   `--InternallyCachedEvaluationRule(Literal: 3.141590e+00)
     |   `--InternallyCachedEvaluationRule(Literal: 2.718280e+00)
     `--InternallyCachedEvaluationRule(Literal: 4.200000e+01)

Printing all of the dependencies known to the evaluator as a GraphViz graph, along with known cached results, e.g.,

digraph Dependencies {
  request_0 -> request_1;
  request_0 -> request_4;
  request_1 -> request_3;
  request_1 -> request_2;
  request_5 -> request_6;
  request_5 -> request_9;
  request_6 -> request_8;
  request_6 -> request_7;
  request_10 -> request_11;
  request_10 -> request_14;
  request_11 -> request_13;
  request_11 -> request_12;

  request_0 [label="ExternallyCachedEvaluationRule(Binary: product)"];
  request_1 [label="ExternallyCachedEvaluationRule(Binary: sum)"];
  request_2 [label="ExternallyCachedEvaluationRule(Literal: 2.718280e+00)"];
  request_3 [label="ExternallyCachedEvaluationRule(Literal: 3.141590e+00)"];
  request_4 [label="ExternallyCachedEvaluationRule(Literal: 4.200000e+01)"];
  request_5 [label="InternallyCachedEvaluationRule(Binary: product) -> 2.461145e+02"];
  request_6 [label="InternallyCachedEvaluationRule(Binary: sum) -> 5.859870e+00"];
  request_7 [label="InternallyCachedEvaluationRule(Literal: 2.718280e+00)"];
  request_8 [label="InternallyCachedEvaluationRule(Literal: 3.141590e+00)"];
  request_9 [label="InternallyCachedEvaluationRule(Literal: 4.200000e+01)"];
  request_10 [label="UncachedEvaluationRule(Binary: product)"];
  request_11 [label="UncachedEvaluationRule(Binary: sum)"];
  request_12 [label="UncachedEvaluationRule(Literal: 2.718280e+00)"];
  request_13 [label="UncachedEvaluationRule(Literal: 3.141590e+00)"];
  request_14 [label="UncachedEvaluationRule(Literal: 4.200000e+01)"];
}

Meant as a replacement for the barely-started iterative type checker, introduce a simpler "evaluator" that can evaluate individual requests (essentially, function objects with some additional API), caching results as appropriate and detecting cycles.

…he cache. Turn the `activeRequests` stack into a `SetVector` and use it to detect cycles. This approach works for uncached requests, and lets us simplify the external-caching protocol quite a bit because we no longer record “in-flight” states. Simplify the cache as well. Thanks to Graydon for the suggestion.

Simplify the static registration of types for use with TypeID by introducing a more declarative approach. Each zone provides a .def file listing the types and templates defined by that zone. The .def file is processed by include/swift/Basic/DefineTypeIDZone.h with its zone number, which assigns values to each of the types/templates and introduces the TypeID specializations.

…ntable. Introduce a CRTP base class, SimpleRequest, which simplifies the task of defining a new request kind by handling the storage of the values (in a std::tuple), their hashing, equality, printing, etc. The values are passed along to the subclass’s operator() so they’re mostly treated as (const) parameters, making mutation of the request state impossible. Extend AnyValue and AnyRequest with printing logic, so we can print any request for debugging purposes, and

Perform online tracking of the (direct) dependencies of each request made to the evaluator, and introduce debugging code to print these dependencies as a tree (from a particular request).

Introduce another form of debugging dump for the evaluator, rendering the complete dependency graph using GraphViz, including all dependencies and values cached within the evaluator.

…Request. The arithmetic evaluator cannot really make use of the new diagnostics machinery, but put it in place for other request kinds.

Simplify the definition of new request kinds by pulling the parameterization of caching logic into SimpleRequest itself, so that the caching-related parts of the contract cannot easily be forgotten.

DougGregor · 2018-06-11T22:19:37Z

@swift-ci please smoke test

DougGregor · 2018-06-12T16:43:17Z

@swift-ci please smoke test and merge

DougGregor · 2018-06-12T17:04:05Z

@swift-ci please smoke test and merge

graydon reviewed Apr 13, 2018

View reviewed changes

huonw reviewed Apr 13, 2018

View reviewed changes

DougGregor force-pushed the evaluator branch from b58bfd9 to a59ad1f Compare April 20, 2018 05:27

DougGregor force-pushed the evaluator branch 2 times, most recently from 3500f16 to 7eb6a97 Compare April 24, 2018 13:54

rudkx reviewed Apr 26, 2018

View reviewed changes

DougGregor force-pushed the evaluator branch from 3a8077f to 19f6b39 Compare May 23, 2018 18:50

DougGregor added 13 commits June 1, 2018 08:56

[Standard library] Remove incorrect FIXME.

03aa3f1

Address the simple issues raised by Graydon and Huon

8886fca

[Evaluator] Track dependencies of each request.

8822b2e

Perform online tracking of the (direct) dependencies of each request made to the evaluator, and introduce debugging code to print these dependencies as a tree (from a particular request).

[Evaluator] GraphViz printing for all of the dependencies.

346bb99

Introduce another form of debugging dump for the evaluator, rendering the complete dependency graph using GraphViz, including all dependencies and values cached within the evaluator.

[Evaluator] Print cached value as part of dependency tree printing.

b12d319

[TypeID Fix comment regarding SWIFT_TYPEID_ZONE

a4db1d0

[ChangeLog] Add an entry for SE-0194: Derived Collection of Enum Cases

6822b79

[Evaluator] Simplify the definition of cycle diagnostics for a Simple…

44099ad

…Request. The arithmetic evaluator cannot really make use of the new diagnostics machinery, but put it in place for other request kinds.

[Evaluator] Add a Caching template parameter to SimpleRequest.

798d60c

Simplify the definition of new request kinds by pulling the parameterization of caching logic into SimpleRequest itself, so that the caching-related parts of the contract cannot easily be forgotten.

DougGregor force-pushed the evaluator branch from b2cf7e1 to 798d60c Compare June 1, 2018 22:11

[Evaluator test] Pull in std::isnan.

f9db209

DougGregor changed the title ~~[WIP][Prototype] Introduce a simple request evaluator.~~ Introduce a simple request evaluator. Jun 12, 2018

DougGregor merged commit 0f2de23 into swiftlang:master Jun 12, 2018

DougGregor deleted the evaluator branch June 12, 2018 17:41


		namespace swift {

		/// Stores a value of any type satisfies a small set of requirements


		struct TypeLoc;

		/// Request the type from the ith entry in the inheritance clause for the

Introduce a simple request evaluator. #15917

Introduce a simple request evaluator. #15917

Uh oh!

Conversation

DougGregor commented Apr 13, 2018

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

huonw Apr 13, 2018 • 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!

DougGregor commented Apr 20, 2018

Uh oh!

huonw commented Apr 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DougGregor commented May 23, 2018

Uh oh!

DougGregor commented Jun 11, 2018

huonw Apr 13, 2018 •

edited

Loading