-
Notifications
You must be signed in to change notification settings - Fork 421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dyno: Initial resolution of zip()
expressions and parallel iterators
#24915
dyno: Initial resolution of zip()
expressions and parallel iterators
#24915
Conversation
I'll fix the CI check failures tomorrow, have to head out for the night. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm halfway through, a few comments, mostly minor changes within individual lines.
Adding |
92a3803
to
24d9319
Compare
My PR has exposed a different bug in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a straggler comment I apparently never submitted.
@@ -819,6 +820,33 @@ void TypedFnSignature::stringify(std::ostream& ss, | |||
ss << ")"; | |||
} | |||
|
|||
bool TypedFnSignature:: | |||
isIterWithIterKind(Context* context, const std::string& iterKindStr) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using UniqueString for this after all because USTR comparisons are O(1)
-- as opposed to O(n)
for std::strings. They also require less allocations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went over it again since I've not been ooking at it for a while. This looks very very slick. 👍
frontend/lib/resolution/Resolver.cpp
Outdated
if (it != m->end()) return it->second; | ||
} | ||
} | ||
return QualifiedType(QualifiedType::VAR, UnknownType::get(rv.context)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For UnknownType
, I think you need to set the kind to QualifiedYype::UNKNOWN
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this would be better supported by a query. Seems like a fair amount of unnecessary work to redo & there will only be a handful of inputs and outputs to it.
frontend/lib/resolution/Resolver.cpp
Outdated
// Resolve iterators, stopping immediately when we get a valid yield type. | ||
auto ret = [&]() -> IterDetails { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you doing this to make early return
possible? I suggest extracting this into a helper and not defining an immediately-invoked lambda.
frontend/lib/resolution/Resolver.cpp
Outdated
bool needStandalone = iterKindStr == "standalone"; | ||
bool needLeader = iterKindStr == "leader"; | ||
bool needFollower = iterKindStr == "follower"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use USTR("standalone")
etc. here to make the comparison O(1) instead of O(n).
((fn->isParallelStandaloneIterator(context) && needStandalone) || | ||
(fn->isParallelLeaderIterator(context) && needLeader) || | ||
(fn->isParallelFollowerIterator(context) && needFollower) || | ||
(fn->isSerialIterator(context) && needSerial)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can this be possible? I don't think we allow being able to write calls with user-provided tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed this - it is possible and is called iterator "forwarding" and is done in the internal modules. It should be an error in user code, though.
cur.enterScope(loop); | ||
if (!ret.isUnknownOrErroneous()) { | ||
rv.handleResolvedCall(iterandRE, astForErr, ci, c, | ||
{ { AssociatedAction::ITERATE, iterand->id() } }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suspect the associated action needs to be different for turning foo()
into foo(tag=follower)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't worry about that right now; we aren't consuming the associated actions yet and will know more when we do. It might be sufficient because the resolved function should be saved here in the associated action.
if (!isIterator()) return false; | ||
|
||
auto ik = types::EnumType::getIterKindType(context); | ||
if (auto m = types::EnumType::getParamConstantsMapOrNull(context, ik)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider early return if this is nullptr
to avoid nesting code.
#define ADVANCE_PRESERVING_SEARCH_PATHS_(ctx__) \ | ||
do { \ | ||
ctx__->advanceToNextRevision(false); \ | ||
setupModuleSearchPaths(ctx__, false, false, {}, {}); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly speaking this doesn't preserve them as much as it resets them to empty. A macro or method for this that truly preserves search paths would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll go ahead and rename the macro to something like "advance preserving standard modules".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, this looks good. I'm happy to see Resolver::enter(const IndexableLoop* loop)
broken up into helper functions.
I'd like you to address my feedback comments before merging; some of these are asking you to restructure some of the code, but I am not expecting these changes to be very hard since the computation will be the same.
Can you take a minute to try to add the early check for it being an array type? I am not expecting that is very hard. If it is hard, listing it as Future Work in the PR message would be good. I think we should try to do this sooner rather than later as I expect it'll be a compilation performance hit if we don't.
@@ -949,6 +955,9 @@ class TypedFnSignature { | |||
const TypedFnSignature* parentFn, | |||
Bitmap formalsInstantiated); | |||
|
|||
bool | |||
isIterWithIterKind(Context* context, UniqueString iterKindStr) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we introduce an enum in the C++ code for this? The use of UniqueStrings is OK with me as well; but an enum would allow additional checking at compile-time (e.g. avoiding mis-spelling; and in some cases we can write switch statements and have the compiler check we covered all cases).
in 'et' to each constant represented as a param value. | ||
If there are multiple enum constants with the same name (which | ||
means the AST is semantically incorrect), then only the first | ||
constant is added to the map. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This docs string should say in what situation it would return nullptr.
frontend/lib/types/EnumType.cpp
Outdated
auto k = UniqueString::get(context, elem->name().str()); | ||
auto it = ret.find(k); | ||
if (it != ret.end()) continue; | ||
ret.emplace_hint(it, std::move(k), std::move(qt)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's clearer to use insert
instead of find/emplace_hint. Also we use this insert
pattern in many other places in dyno, so using it here would make this code easier to follow for anybody looking at lots of dyno code.
insert
can return a pair, where the second element of that pair indicates if insertion took place. If the map already had the element, insert
won't insert anything, and so that second element will be false
.
|
||
auto it = m->find(iterKindStr); | ||
if (it != m->end()) { | ||
bool isFollowerIterKind = iterKindStr == "follower"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iterKindStr == USTR("follower");
@@ -843,6 +844,33 @@ void TypedFnSignature::stringify(std::ostream& ss, | |||
ss << ")"; | |||
} | |||
|
|||
bool TypedFnSignature:: | |||
isIterWithIterKind(Context* context, UniqueString iterKindStr) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be unnecessarily slow in the context of isSerialIterator
where it is called multiple times with different kind strings.
Let's instead make a function to return the iterator kind for iterators. Then the TypedFnSignature would be doing things like iterKind(context) == USTR("leader")
.
Also, IMO it is worthwhile to make some kind of representation of the iterator kind part of TypedFnSignature
itself. That would prevent the work of repeatedly computing this information. I don't view this as a strict requirement, but if it appeals to you, let's go ahead with it. (If you do this, please add an enum for the compiler's representation of iterator kinds). Of course, an alternative way would be use a query to compute the iterKind for a const TypedFnSignature*
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am torn about whether or not to add an enum or just compute it. I'm not sure the performance cost of computing it repeatedly will be that high or significant. I agree this particular code can be improved.
Context* context = rv.context; | ||
|
||
if (mask == IterDetails::NONE || rv.scopeResolveOnly) { | ||
iterand->traverse(rv); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably deserves a comment here about why we need to traverse the iterand in this case.
MSC.only().fn()->untyped()->kind() == uast::Function::Kind::ITER; | ||
// Resolve the iterand but suppress errors for now. We'll reissue them | ||
// next, possibly suppressing a "NoMatchingCandidates" for the iterand if | ||
// our injected call is successful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why suppress errors here? That seems odd to me. I would think, if we have for i in abc()
or forall j in def()
then an error in resolving abc()
or def()
would be fatal. Is the issue that, def
might have a standalone iterator but not a serial iterator? Would that be better handled by resolving interior parts of a call (e.g. forall j in a(b(c()))
we resolve b(c())
but delay resolving a()
for now.
It seems to me that we could do this by simply traversing the children of iterand
here before we proceed. The runAndTrackErrors
stuff is cool but I think it has performance implications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just needed to resolve the iterand's type fully before I could make any decisions about it. It could be an iterator call. It could be as you say, a type with a standalone iterator but not a serial, in which case we need to inject the arguments and try again.
This is one way to do it, but I agree about the potential negative performance impact. Another option could be to add some sort of bool doNotEmitCallErrors
flag to the Resolver
. I tried to get that to work for a bit but I wasn't happy with it.
I will add a future work item about exploring an alternative to runAndTrackErrors
.
bool wasIterandTypeResolved = !iterandRE.type().isUnknownOrErroneous(); | ||
bool wasIterResolved = fn && fn->isIterator(); | ||
bool wasMatchingIterResolved = wasIterResolved && | ||
((fn->isParallelStandaloneIterator(context) && needStandalone) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this code would be more efficiently written if it computes the iterator tag and then does comparisons including needStandalone etc.
auto iteratorTag = fn->iteratorTag(context);
if ((iteratorTag == USTR("standalone") && needStandalone) || ...
(of course it would look different if you add that enum).
cur.enterScope(loop); | ||
if (!ret.isUnknownOrErroneous()) { | ||
rv.handleResolvedCall(iterandRE, astForErr, ci, c, | ||
{ { AssociatedAction::ITERATE, iterand->id() } }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't worry about that right now; we aren't consuming the associated actions yet and will know more when we do. It might be sufficient because the resolved function should be saved here in the associated action.
frontend/lib/resolution/Resolver.cpp
Outdated
|
||
// TODO: What would it take to make backpatching of array types happen | ||
// _before_ / without resolving an iterator? Currently we rely on iterator | ||
// resolution to resolve the iterand for us. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that we only needed to detect if elements yielded by the loop expression are types rather than values? Also it checks something about there being a domain involved. Seems like it'd be easy enough to filter these earlier. Am I wrong about something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change the stale TODO
.
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
While here, also comment out the 'testHelloWorld' dyno test. Resolving parallel iterators has introduced some bugs that cause this test to fail. Re-add it when we think the dyno resolver is ready. Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
8f5f415
to
b509796
Compare
Cool! |
…25994) This PR adds a new type sub-hierarchy for iterable things. It creates some scaffolding for promoted expressions, though it does not currently implement them. It also matches production's implementation of iterators, which, as we have discussed with @bradcray, @mppf and @benharsh may not be the direction we want to take in the long term. ## What? This PR adds a new `IteratorType` type, as well as subclasses for it for various situations that produce iterables (promotion, loop expressions, calls to `iter` procedures). This way, we can unify the logic for resolving iterators on records/classes and "iterables", invoking a `these` method. The `these` methods for user records/classes are, as before, provided explicitly by the user; the `these` methods for iterators are compiled-provided (production generates them, in Dyno we simply have special logic to mimic their resolution). The three subclasses of `IteratorType` are as follows: * `FnIteratorType`: the class of iterators created from invocations to `iter` procedures. Just like in production, this type contains a reference to the function that created it. When the iterator is traversed (using `these`), the compiler resolves a call to a function with the same name as the original `iter` procedures, and with the same formal types. This matches the production compiler's implementation, down to the ability to provide additional overloads for serial/parallel iterators outside of where they are originally defined (for better or for worse). * `LoopExprIteratorType`: the class of iterators created from loop expressions. In production, there are some tricky cases that make the "yield type" of a loop expression be fluid, since it can depend on the instantiation of the follower iterator it uses, which can change depending on the leader. Thus, it's possible that in `forall i in (loopExpr)` and `forall (_, i) in zip(.., loopExpr)`, the index variable `i` is a different type. To make this work, we could need to capture the outer variables of the loop expression and re-resolve it with different leaders. This seems undesirable as per discussions with Brad et. al., and as a result, I instead make the assumption that loop expressions always yield the same type, and that they do not re-resolve their leaders and followers. There's some future work to make this more principled. * `PromotionIteratorType`: the class of iterators created from promoted expressions. Since promotion is not implemented, this is just left as a scaffold. This PR removes some speculative resolution logic originally introduced by @dlongnecke-cray in #24915. Specifically, calls to iterators like `foo()` are no longer re-resolved with tags if original resolution fails. This has several motivators: 1. Production doesn't support this, and instead requires a serial iterator to be present for `foo()` to successfully invoke `foo(standalone)` etc. 2. In David's PR, the re-resolution only happens at the site of `zip`s / loop iterands. However, if we truly wanted to implement this feature, we would need to handle invocations of iterators in arbitrary contexts (e.g., as sub-expressions of promoted expressions), and thus would likely need to hoist the "insert tags if needed" logic to the more general call resolution world. 3. The need to speculatively resolve an expression before inserting tags (and re-emit errors later if needed) made it difficult to resolve arrays types early (instead of resolving them as bracket loops). * Resolving array types late no longer works under this PR, but I think that's fine. Previously, we would resolve a call to `_domain.these` with a generic receiver (normally not allowed), and fail to infer its return type despite succeeding in call resolution. This PR requires knowing the return type (to make use of the fact that it's iterable), so this no longer flies. Anyway, relying on resolving call with an accidentally generic receiver seems unfortunate. * Resolving array types early also means we avoid unnecessary invocations of 'these' which is a nice bonus. ## Future Work Following some discussion, we'd like to change the following properties of the loop expressions (at least): 1. The return type may not depend on the type of the leader, or on whether the serial or parallel overload was called. This way, loop expressions _will_ always have a consistent yield type, no matter the context in which they are used. 2. Implementing (1) would still allow a different follower iterator to be resolved depending on the leader type (it would just need to return a value of the right type). We would like to rule out this "context dependence", and make it so the same follower function is invoked for any given iterator expression. * This would likely require eagerly resolving the leader/follower of the iterator in the place where it is found, and then re-using these resolve calls whereever needed. * Loop expressons that need one follower type but are used with a leader type yielding other things will be rejected. 3. We do find it desirable for `foo()` to be able to invoke `foo(standalone)` even if `foo()` doesn't exist. This PR removes David's original form of this, but does not yet provide a replacement. Reviewed by @dlongnecke-cray and @mppf -- thanks! # Testing - [x] dyno tests
This PR adds support to dyno for resolving promotion. This is achieved by matching the production implementation in most ways, which boils down to allowing a `chpl__promotionType()` function to be defined for promotion-eligible data structures. The resolution process then takes this as a hint to try to perform promotion if other candidates don't match. There are numerous changes in this PR (though a bulk of the diff is actually related to reporting errors from iteration, and to the tests I wrote). 1. __Allowed `resolveCall` to not accept any scope arguments (no POI scope, no call scope).__ You may see this change reflected in the moving-around of `if (scope)` conditionals. The purpose of this was to re-use the existing logic for searching the receiver types (including parent classes etc.), which is intermingled with the logic for searching regular scopes in which the call is occurring. When resolving `chpl__promotionType()`, we do not care about the call scope because of the next bullet point. 2. __Departed from using instantiation scopes for searching for `chpl__promotionType`.__ In production, we track "instantiation points" of types, which unfortunately end up being the places where any given concrete instantiation is __first created__ (it can be created in many places, e.g., if the user instantiates a generic type `R` in two distinct blocks). As a result, there has been some weirdness with finding the `chpl__promotionType` in odd places. This PR departs from this logic by only using the receiver type's definition scope as an anchor, which seems to me to be quite reasonable. 3. __Put the existing logic for `canPass` into a `canPassScalar` function, which does not consider promotion.__ This way, I could adjust several call sites to entirely circumvent the need for attempting to resolve `chpl__promotionType`, because in those call sites promotion can't occur. The new `canPass` simply invokes `canPassScalar`, and if that fails, attempts to find the promotion type and re-run `canPassScalar` with that. 4. __Stored promoted formals in `MostSpecificCandidate` to distinguish the case of a serial function matching from the case of promotion.__ This helps determine whether the return type of a call is the scalar type of the iterator record corresponding to the promotion. The map of promoted formals is copied into the `PromotionIteratorType` when it is constructed. 5. __Adjusted `DisambiguationCandidate` to always compute the conversion counts etc., which is also where the promotion counts are determined.__ Doing so made it possible to extract the promoted formals and store them into `MostSpecificCandidates` as part of the resolution process. While there, made the `computeConversionInfo` free function into a method, which cleaned up some code. 6. __Made the `resolveTheseCall` family of functions return a new type called a `TheseResolutionResult`__. This is a type that wraps a `CallResolutionResult` and provides additional information that helps print error messages during resolution. Specifically, this new type contains a reason for failure (e.g., "couldn't find iterator"), the offending call resolution result (if any), as well as potentially a nested `TheseResolutionResult` if the failure was caused by zippering (so that we can say "we couldn't resolve this zippered iteration, because the nth argument didn't have a serial iterator"). 7. __Adjusted the code for resolving `zip` as part of `Resolver.cpp` to emit error messages cohesively__ (as opposed to individual error messages for failed zippered args). To this end, I added a mechanism to note various `TheseResolutionResults` corresponding to various cases (e.g., "`for` loop doesn't support serial iterators", or "the nth argument doesn't have a follower", etc.) and then aggregate them into a `NonIterable` error (see next bullet point). This helps emit very pretty error messages for when things go wrong. <img width="1173" alt="Screen Shot 2024-11-04 at 10 45 36 AM" src="https://github.com/user-attachments/assets/85eccec8-c9ef-4846-b42d-d2035a5043ed"> <img width="1247" alt="Screen Shot 2024-11-04 at 10 44 46 AM" src="https://github.com/user-attachments/assets/90495166-ea72-4413-8d52-ee0fee3eda17"> 8. Drastically improved the `NonIterable` error message. By passing `TheseResolutionResults` as arguments, we can now provide very detailed explanations for why each iterator resolution attempt was unsuccessful. This was the point of `TheseResolutionResult`. Minor changes: * Added `const` to `mark` instance of `unordered_map` to allow marking constant maps. * Removed unnecessary newline when printing the words "unknown type". No idea how that got there. * Fixed a bug in which disambiguating based on conversions for functions with `int` formals caused a crash (because for some reason, `params` were expected). Reviewed by @benharsh -- thanks! ## Future Work @bradcray mentioned that the reason promoted expressions and loop expressions don't have standalone iterators is likely due to chronological / historical reasons, and that we should consider adding them in Dyno. This is relatively easy, but this PR is big and I would like to leave that as a follow-up. This PR maintains (and even defines a nicer error for) the behavior that @dlongnecke-cray introduced for resolving zippered expression in #24915 (specifically, that we "commit" to either a leader/follower or serial strategy after inspecting the first iterand of a zip expression). This can lead to trouble if the first zippered argument has a leader, but the subsequent arguments don't have followers. In this case, a serial-capable loop (e.g., bracket loop) could iterate serially, but won't, because we attempted to resolve the leader/follower and committed to it. Production doesn't behave this way, so a subsequent PR ought to address this. This PR also notably does not address the case in which __nested__ promoted functions or iterators are invoked. This is noted where the trivial (and thus, incomplete) `ResolutionContext` is created in `getPromotionType`. Perhaps @dlongnecke-cray could offer some advice there in future PRs. ## Testing - [x] dyno tests, including ~40 new ones for promotion
This PR introduces resolution of
zip()
expressions and resolution of parallel iterators forforall
and[]
loops.For
zip()
expressions that appear as the iterand of a serial loop, the strategy is to resolve only serial iterators for each actual in thezip()
.For
zip()
expressions that appear as the iterand of aforall
loop, the strategy is to:For
[]
loops, the strategy is similar, except serial iterators may be used as a substitute for the leader and followers IFF the leader iterator could not be resolved for the leader. If the leader iterator could be resolved, but e.g., its return type could not, then serial iterators will not be considered as fallbacks.For iterands that are not
zip()
expressions, the "standalone" parallel iterator is preferred for parallel loops before attempting to resolve a leader/follower combo. As withzip()
,forall
loops will emit an error if no form of parallel iterator could be resolved. All other loops will fall back to serial iterators.Thanks to @vasslitvinov for walking me through the semantics of
zip()
and parallel iterator resolution.FUTURE WORK
tag=tag
,followThis=followThis
)TypedFnSignature
expressing itsiterKind
Reviewed by @DanilaFe, @mppf. Thanks!