Skip to content
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

Avoid redundancy with resize ops #3713

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Avoid redundancy with resize ops #3713

wants to merge 1 commit into from

Conversation

naoyam
Copy link
Collaborator

@naoyam naoyam commented Jan 15, 2025

Closes #3710

@naoyam
Copy link
Collaborator Author

naoyam commented Jan 15, 2025

!test

Copy link

github-actions bot commented Jan 15, 2025

PR Reviewer Guide 🔍

(Review updated until commit 9b7cda6)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
⚡ Recommended focus areas for review

Logic Change

The function getPredicates has been modified to predicate the input ID of a resize operation. This change may have unintended consequences and should be thoroughly reviewed.

std::vector<IterDomain*> predicate_domains = getPredicateDomains(tv, expr);

const IndexingInfo& index_info =
    computeIndex(expr, predicate_domains, for_loops);

const auto& index_map = index_info.index_map;

const std::unordered_map<Val*, Val*> replacement_map_start =
    getPredicateIndexReplacementMap(
        tv,
        for_loops,
        index_map,
        traversalGraph(),
        index_info.traversal_path,
        id_model_,
        /*is_start_predicate=*/true,
        /*unswitched_loop=*/unswitched_loop);

const std::unordered_map<Val*, Val*> replacement_map_stop =
    getPredicateIndexReplacementMap(
        tv,
        for_loops,
        index_map,
        traversalGraph(),
        index_info.traversal_path,
        id_model_,
        /*is_start_predicate=*/false,
        /*unswitched_loop=*/unswitched_loop);

// When resize is involved, predicate its input ID as well to avoid
// redudancy. This is only necessary if a predicated resize is
// preceded by a split, however, for now it's always predicated
// with an exception of static resize. See
// PredicateIndexingTest.SplitThenPad for a concrete example.
for (const auto& [eg, direction] : index_info.traversal_path) {
  auto resize = dynamic_cast<Resize*>(eg->front());
  if (resize == nullptr) {
    continue;
  }

  // TODO: It seems this shouldn't be predicated when the direction is
  // Forward, i.e., when resize ops are propagated from
  // producers to consumers. For example, ResizeTest.SliceThenPadLeftHalf
  // would fail with this. Revisit for the Forward case if necessary.
  if (direction == Direction::Forward) {
    continue;
  }

  // If the input ID is guaranteed to cover the output ID, then
  // the input index should never exceed its boundary.
  if (resize->leftExpand()->isConstInt() &&
      resize->rightExpand()->isConstInt()) {
    auto left_int = resize->leftExpand()->evaluate().as<int64_t>();
    auto right_int = resize->rightExpand()->evaluate().as<int64_t>();
    // If the traversal direction is forward, the predicate is not
    // necessary if both of the left and right factors are
    // non-negative as the ouput ID is guaranteed to cover the
    // input ID. Similarly, if it's backward, it is not necessary
    // if they are non-positive.
    if ((direction == Direction::Forward && left_int >= 0 &&
         right_int >= 0) ||
        (direction == Direction::Backward && left_int <= 0 &&
         right_int <= 0)) {
      continue;
    }
  }

  IterDomain* id_to_predicate =
      direction == Direction::Forward ? resize->out() : resize->in();

  predicate_domains.push_back(id_to_predicate);
}
Potential Bug

The TODO comment on line 1107 suggests that the current implementation may not be correct for the Forward direction. This should be investigated and fixed.

// Forward, i.e., when resize ops are propagated from
// producers to consumers. For example, ResizeTest.SliceThenPadLeftHalf
// would fail with this. Revisit for the Forward case if necessary.
if (direction == Direction::Forward) {
  continue;
}

@naoyam
Copy link
Collaborator Author

naoyam commented Jan 17, 2025

!test

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.

Redundant accesses due to missing predicates in resize
1 participant