Skip to content

Resolve broadcasts resulting from a PadOp#610

Merged
jacobhinkle merged 47 commits intomainfrom
resolve_resize_broadcasts
Sep 27, 2023
Merged

Resolve broadcasts resulting from a PadOp#610
jacobhinkle merged 47 commits intomainfrom
resolve_resize_broadcasts

Conversation

@jacobhinkle
Copy link
Collaborator

@jacobhinkle jacobhinkle commented Jul 18, 2023

This changes broadcast analysis to drop the assumption that all broadcasts are created through a BroadcastOp. This change allows a broadcast to be introduced between root and rfactor of any tensor, which happens if we pad an Iteration domain with negative pad widths, resulting in a Broadcast domain.

This adds a map_symbolic option to PairwiseRootDomainMap, which defaults to false and has the following effect (condition 5):

  • When map_symbolic == false, symbolic IterDomains are only mapped with one another if their extent expressions match (sameAs). This is the default mode and safely handles cases where one symbolic value will concretize to broadcast that is resolved by the other.
  • When map_symbolic == true, symbolic IterDomains are mapped regardless of their extent expressions. This mode is useful for producer to consumer mappings when we know we are not resolving a broadcast, but a placeholder extent expression might be used in the consumer. It is only used for propagating concretizations from producer to consumer, in which case we need to exact map aligned symbolic IDs.

Fixes #596. Fixes #357

@jacobhinkle
Copy link
Collaborator Author

Dynamic test is currently failing in ExpressionEvaluator::propagateBoundValuesThroughExactMaps due to #357 which we should probably finally address.

Comment on lines +186 to +194
// Condition 5
// At least one ID is symbolic.
// Map producer to consumer if and only if their extents are identical
if ((producer_id->isSymbolic() || consumer_id->isSymbolic()) &&
(!producer_id->extent()->sameAs(consumer_id->extent()))) {
itc++;
itp++;
continue;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This replaces earlier iterations on handling exact mapping of Symbolic domains, which I think were more cumbersome. The meaning of a Symbolic IterType is that it could potentially be a Broadcast domain. This is essentially reflecting that the extent expression hasn't been evaluated yet. If we have identical extents for two aligned Symbolic IterDomains, this should mean they will have the same IterType after concretization. In that case, they should be exact mapped. This lets us propagate bound extents in ExpressionEvaluator much more easily, while still avoiding exact mapping between transformed symbolic domains.

Comment on lines +731 to +736
std::vector<std::unordered_map<IterDomain*, IterDomain*>> c2p_maps;
for (auto producer : ir_utils::filterByType<TensorView>(def->inputs())) {
PairwiseRootDomainMap root_map(producer, consumer);
c2p_maps.push_back(
root_map.mapConsumerToProducer(consumer->domain(), producer->domain()));
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precomputing c2p maps so they don't need to be recomputed for each consumer ID/producer pair.

@jacobhinkle jacobhinkle marked this pull request as ready for review September 13, 2023 16:17
@jacobhinkle jacobhinkle requested a review from naoyam September 13, 2023 16:29
@jacobhinkle
Copy link
Collaborator Author

!build

Copy link
Collaborator

@naoyam naoyam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the fix!

@jacobhinkle jacobhinkle merged commit c09cf93 into main Sep 27, 2023
@jacobhinkle jacobhinkle deleted the resolve_resize_broadcasts branch September 27, 2023 00:39
jacobhinkle added a commit that referenced this pull request Oct 4, 2023
)

Stacked on #610; see
#876 (comment).

This PR:

- Changes the `analyzeResizes` pass in concretization to inspect
expanded extents
- Changes root->rfactor propagation in concretization to return early
when IterType is already concretized. This avoids propagating
`Broadcast` in this test when we have already marked the resized ID as
`Iteration`.
- Changes `PrecomputedValues::bindInputs` to bind not only metadata but
also the actual `TensorView` arguments.

I noticed that the `ExpressionEvaluator` used during compilation
contained more bound scalars than the one used at execution where we
fail to evaluate the extent. We had `i0` and `i2` bound at execution,
but we did not have `T0` bound, so we could not compute
`getMetaData(T0)`. At compilation, `T0` was bound so there was no
problem until execution.

Note that at compilation, we use `auto expr_eval =
executor_utils::bindInputs(args, kernel);` whereas at compilation we use
`evaluatorPrecomputedValues()->bindInputs(args);`. The difference is
that `PrecomputedValues::bindInputs` will call `bindTensorMetaData`
instead of binding the actual tensor. This PR also binds the actual
tensor in addition to its metadata in that method.

Fixes #870

---------

Co-authored-by: Gao, Xiang <qasdfgtyuiop@gmail.com>
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.

Static pad to broadcast fails to schedule Symbolic iter domains should not exact map when aligned

3 participants