Skip to content

Fix contiguity of expanded tensor#2049

Merged
zasdfgbnm merged 8 commits intodevelfrom
fix-expand-contiguity
Oct 12, 2022
Merged

Fix contiguity of expanded tensor#2049
zasdfgbnm merged 8 commits intodevelfrom
fix-expand-contiguity

Conversation

@zasdfgbnm
Copy link
Copy Markdown
Collaborator

@zasdfgbnm zasdfgbnm commented Oct 11, 2022

  • Make TensorViewBuilder able to generate expanded tensor.
  • Fixes a problem on the contiguity found while working on Fix vec validation on expanded broadcasting #2044. Note that an expanded dimension and the dimension before can never be contiguous. Without fixing this, the added test will generate silent wrong result.
  • Validate contiguity of expanded dimensions during lowering

@zasdfgbnm zasdfgbnm requested review from csarofeen and naoyam October 11, 2022 08:30
Val* expanded_extent = nullptr;
Val** shape_extent = &extent;
if (!expanded_.empty()) {
is_expanded = expanded_[i];
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just in case, please use expanded_.at(i) to avoid silent memory errors

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

fixed

if (i == -1) {
shape_.emplace_back(IrBuilder::create<Int>());
} else if (i == 1) {
shape_.emplace_back(FusionGuard::getCurFusion()->oneVal());
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there any specific reason to use the one val? Do we also want to use the zero val?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Just trying to save an instance creation; I don't think it will make much difference here. And yes, we should also use zero val, because why not.

domain[i] =
IterDomainBuilder(FusionGuard::getCurFusion()->zeroVal(), shape_[i])
.build();
*shape_extent = shape_[i];
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same here

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@naoyam
Copy link
Copy Markdown
Collaborator

naoyam commented Oct 11, 2022

Contiguity and expanded broadcasts are still making me feel uneasy as I've yet got a clear idea of what they mean in PyTorch. Can you please explain why expanded domains and their left domains should never be contiguous?

@zasdfgbnm
Copy link
Copy Markdown
Collaborator Author

@naoyam For example, if you have a contiguous tensor of shape (4, 5, 1, 6), then the stride could be (30, 6, whatever, 1), but if you expand this tensor to (4, 5, 10, 6), the stride has to be (30, 6, 0, 1). And by definition, if we have true contiguity at the expanded dimension (dim 2), its stride must be stride[3] * 6, which can not be 0. Similarly, for the dimension before the expanded dimension (dim 1), if it had true contiguity, then its stride must be stride[2] * 10, which is always zero.

@naoyam
Copy link
Copy Markdown
Collaborator

naoyam commented Oct 11, 2022

@naoyam For example, if you have a contiguous tensor of shape (4, 5, 1, 6), then the stride could be (30, 6, whatever, 1), but if you expand this tensor to (4, 5, 10, 6), the stride has to be (30, 6, 0, 1). And by definition, if we have true contiguity at the expanded dimension (dim 2), its stride must be stride[3] * 6, which can not be 0. Similarly, for the dimension before the expanded dimension (dim 1), if it had true contiguity, then its stride must be stride[2] * 10, which is always zero.

Thanks for the explanation. Can you please add this as a comment to the code? Maybe at TensorDomain::getContiguousContiguity?

@zasdfgbnm
Copy link
Copy Markdown
Collaborator Author

@naoyam I have resolved all review comments

Copy link
Copy Markdown
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.

2 participants