Drop contiguity info for size 1 dimensions for Python's define_tensor(sizes=..., strides=...)#2551
Drop contiguity info for size 1 dimensions for Python's define_tensor(sizes=..., strides=...)#2551IvanYashchuk wants to merge 4 commits intocsarofeen:develfrom
Conversation
…(sizes=, strides=)
| " is not supported in nvFuser. Expected size > 0."); | ||
| if (sizes[i] == 1) { | ||
| maybe_symbolic_sizes.push_back(1); | ||
| contiguity.erase(contiguity.begin() + i); |
There was a problem hiding this comment.
Strange. Isn't this already handled by computeContiguity?
There was a problem hiding this comment.
Here, in maybe_symbolic_sizes, there's a specialization on size 1 dimensions independent of strides, while computeContiguity checks stride==0 for the dimension to be considered as the broadcast dimension.
Maybe that line should be changed to auto not_broadcast = [&](auto i) { return sizes[i] != 1; }; ?
There was a problem hiding this comment.
Ahh, I see. I am OK with changing not_broadcast for now to unblock. But I think a better solution is to a some refactor on the python frontend to handle size N stride 0 dimension as expanded broadcast.
There was a problem hiding this comment.
#2561 handles size N stride 0. Should we move along with that instead of this PR?
| "compute_contiguity: Sizes and strides must have the same number of dimensions"); | ||
| auto not_broadcast = [&](auto i) { return strides[i] != 0 && sizes[i] != 1; }; | ||
| auto not_broadcast = [&](auto i) { | ||
| return (sizes[i] != 1) || (strides[i] != 0 && sizes[i] != 1); |
There was a problem hiding this comment.
Probably this should be sizes[i] != 1? test_compute_contiguity might fail if we do so, but we can skip that test for now.
|
Not trying to block this PR. think about a tensor We still have issue with explicitly broadcast dimensions that I don't think is handled by this PR yet. With the API on TensorOpRecord, we can't properly pass correct contiguity information for an expanded dimension. Wondering if you want to patch that in this PR as well? otherwise I'll start one once this merges. |
|
Closing in favor of #2561 |
Fixes #2549.