Skip to content

Concretize dynamic expressions in topological order#760

Closed
jacobhinkle wants to merge 11 commits intomainfrom
concretize_in_order
Closed

Concretize dynamic expressions in topological order#760
jacobhinkle wants to merge 11 commits intomainfrom
concretize_in_order

Conversation

@jacobhinkle
Copy link
Collaborator

@jacobhinkle jacobhinkle commented Aug 22, 2023

During concretization we currently analyze all dynamic reshape operations, then we analyze dynamic resize operations. For each op, we need to evaluate the output extents, so those extents must be such that an ExpressionEvaluator can evaluate them following propagateBoundValuesThroughExactMaps. That can lead to some confusion since we can either use placeholder extents and try to propagate through symbolic aligned IDs which might not exact map after concretization (see #357) or try to always form exactly accurate extent expressions at definition. The latter approach gets complicated quickly when those expressions explode in complexity, such as with slice (see #460 (comment)). The former approach is preferred but is fragile since it requires exact mapping Symbolic domains sometimes but not others, violating #357.

A way forward is to use placeholder extents instead of exact expressions, then instead of altering PairwiseRootDomainMap to redefine exact mapping of symbolic domains, we can bind the appropriate placeholder expressions in the ExpressionEvaluator manually when analyzing dynamic expressions in concretization. Using more placeholders instead of raw extent expressions (not yet implemented in this PR) will hopefully also help prevent losing scalars during segmentation (e.g. #656). This approach requires us to analyze those expressions in topological order, which this PR implements.

This PR alters DynamicTransformInitialInfo:

  • Removes dynamic_reshaped_tvs_ and dynamic_resized_ids_.
  • Introduces dynamic_exprs_ which currently holds outputs of ViewOp or Resize exprs.

It then loops over dynamic_exprs_ which is topologically ordered when doing the analysis to build DynamicTransformConcretizationInfo. This PR does not implement manual binding in the ExpressionEvaluator as it is not yet needed; a future PR (perhaps before #511) will implement that using definition-time placeholder extents introduced for consumer root domains of Symbolic domains in newOutputDomain.

This PR also alters DynamicTransformConcretizationInfo:

  • Removes reshape_transforms_ and resize_itertypes_.
  • Introduces concretization_descriptors_, which is a vector of std::variants holding concretization information for each dynamic expression in reverse order.

To perform the concretization, we traverse the dynamic expressions in reverse topological order. The order is reversed since we need to replace Exprs which can lead to difficulties dereferencing dead pointers if done in forward order.

Note that our IR does not represent TensorViews as dependencies of IterDomains, so IterDomain expressions are handled when we visit their associated TensorView. See the comment at DynamicTransformInitialInfoBuilder::handle(TensorView*).

@jacobhinkle jacobhinkle changed the title Analyze dynamic expressions in topological order Concretize dynamic expressions in topological order Aug 22, 2023
@jacobhinkle jacobhinkle marked this pull request as ready for review August 29, 2023 15:48
@jacobhinkle jacobhinkle requested a review from naoyam August 29, 2023 15:48
@naoyam
Copy link
Collaborator

naoyam commented Sep 5, 2023

A way forward is to use placeholder extents instead of exact expressions

Is this done in this PR?

@jacobhinkle
Copy link
Collaborator Author

A way forward is to use placeholder extents instead of exact expressions

Is this done in this PR?

It's not. I have another PR that does that. It is a bit more involved so I wanted to separate out this PR, which should not change behavior too much. I changed the comment to reflect that it's not implemented yet.

@jacobhinkle
Copy link
Collaborator Author

I think we should actually not need this PR. Instead, I think the rule of thumb should be that at definition we should always assign accurate, computable scalars as output extents. For pad and cat, we already do this. For reshape we do as well since we use the original scalars provided for the new shape. For slice, we currently assume the simplest branch; instead we should use the accurate but complicated expressions for output extents then simplify them during or after concretization. Closing this PR.

@jacobhinkle jacobhinkle deleted the concretize_in_order branch September 27, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants