Symintify pad ops #87046
Symintify pad ops #87046
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/87046
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 Failures, 1 PendingAs of commit e07d99c: The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
github-actions
bot
requested review from
Chillee,
Krovatkin,
anjali411,
antoniojkim,
bdhirsh,
ezyang,
miladm and
wconstab
[ghstack-poisoned]
| negated_pad.cbegin(), | ||
| negated_pad.cend(), | ||
| negated_pad.begin(), | ||
| std::negate<int64_t>()); |
There was a problem hiding this comment.
We should add a std::negate specialization for SymInt. We should also make operator- work. With the branch, there isn't any particular reason to rush this
aten/src/ATen/native/PadNd.cpp
Outdated
| out_slice = out.slice(dim, out_shape[dim] - pad_r, out_shape[dim]); | ||
| in_slice = out.slice(dim, std::max(pad_l, zero), std::max(pad_l, zero) + pad_r); | ||
| out_slice = out.slice_symint(dim, out_shape[dim] - pad_r, out_shape[dim]); | ||
| in_slice = out.slice_symint(dim, pad_l.max(zero), pad_l.max(zero) + pad_r); |
There was a problem hiding this comment.
We should have a SymInt specialization for std::max
There was a problem hiding this comment.
I added one last week but ended up removing it (oh actually I didn't add a specialization - I just implemented SymInt::min/max)
From the docs, it looks like std::max/min() already have a template that just calls into operator< and operator>. So adding a special handling for min/max on symints in C++ seemed redundant to me, but lmk if you think that's wrong. We still have min/max support in python.
There was a problem hiding this comment.
ok, I am fine with this resolution
Following comments below, we need to add support for `std::negate`/`std::min`/`std::max`/`operator-` for SymInt [ghstack-poisoned]
Following comments below, we need to add support for `std::negate`/`std::min`/`std::max`/`operator-` for SymInt [ghstack-poisoned]
Following comments below, we need to add support for `std::negate`/`std::min`/`std::max`/`operator-` for SymInt [ghstack-poisoned]
Following comments below, we need to add support for `std::negate`/`std::min`/`std::max`/`operator-` for SymInt [ghstack-poisoned]
Following comments below, we need to add support for `std::negate`/`std::min`/`std::max`/`operator-` for SymInt [ghstack-poisoned]
Following comments below, we need to add support for `std::negate`/`std::min`/`std::max`/`operator-` for SymInt [ghstack-poisoned]
|
@ezyang this is ready for a final review |
Following comments below, we need to add support for `std::negate`/`std::min`/`std::max`/`operator-` for SymInt [ghstack-poisoned]
|
@pytorchbot merge -g |
Merge startedYour change will be merged once all checks on your PR pass since you used the green (-g) flag (ETA: 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: 2 additional jobs have failed, first few of them are: trunk ,trunk / linux-bionic-cuda11.7-py3.10-gcc7 / test (default, 3, 4, linux.4xlarge.nvidia.gpu) Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -g |
Merge startedYour change will be merged once all checks on your PR pass since you used the green (-g) flag (ETA: 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
|
Hey @albanD. |
Stack from ghstack (oldest at bottom):
Following comments below, we need to add support for
std::negate/std::min/std::max/operator-for SymInt