Skip to content

Conversation

zasdfgbnm
Copy link
Collaborator

@zasdfgbnm zasdfgbnm commented Sep 5, 2025

The current TensorDomain::setContiguity does not correctly handle reduction ID.

Copy link

github-actions bot commented Sep 5, 2025

Description

  • Fix contiguity validation in TensorDomain::setContiguity

  • Replace manual checks with validateContiguity function call

  • Enforce broadcast dimension contiguity rules correctly


Changes walkthrough 📝

Relevant files
Bug fix
nodes.cpp
Refactor contiguity validation in TensorDomain                     

csrc/ir/nodes.cpp

  • Replace manual contiguity validation with validateContiguity call
  • Remove outdated NVF_ERROR and NVF_CHECK assertions
  • Ensure broadcast dimensions have no contiguity value
  • Simplify setContiguity logic using centralized validation
  • +1/-10   

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Possible Issue

    The new code calls validateContiguity(maybeAllocation(), contig) but does not verify that the size of maybeAllocation() matches the size of contig, which was previously enforced by NVF_ERROR. This could lead to undefined behavior if the sizes mismatch.

    validateContiguity(maybeAllocation(), contig);
    Missing Validation

    The previous implementation checked that the contiguity of broadcast dimensions is None and non-broadcast dimensions have a defined contiguity (true/false). This validation is removed without evidence it is handled elsewhere, risking incorrect contiguity assignments.

    validateContiguity(maybeAllocation(), contig);

    "Invalid size of contiguity vector");
    for (auto i : arange(contig.size())) {
    NVF_CHECK(
    maybeAllocation().at(i)->isBroadcast() != contig.at(i).has_value(),
    Copy link
    Collaborator Author

    @zasdfgbnm zasdfgbnm Sep 5, 2025

    Choose a reason for hiding this comment

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

    This should be:

    (maybeAllocation().at(i)->isBroadcast() || maybeAllocation().at(i)->isReduction()) != contig.at(i).has_value()

    @zasdfgbnm
    Copy link
    Collaborator Author

    !test

    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

    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.

    3 participants