Skip to content

Use __cpp_aligned_new instead of hand-rolling the implementation #11293

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 4, 2025
Merged

Use __cpp_aligned_new instead of hand-rolling the implementation #11293

merged 1 commit into from
Jun 4, 2025

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Jun 2, 2025 •
edited
Loading

The current feature detection seems to be incorrect, resulting in fallback to
the manual implementation, which in turn returns untagged pointers under
hwasan.
Use new expressions instead to delegate the work to the C++ compiler.

Differential Revision: D75806635

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Jun 2, 2025 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/11293

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit f7fbffe with merge base bca2cf5 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported labels Jun 2, 2025
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75806635

@tamird
Copy link
Contributor Author

tamird commented Jun 2, 2025

@pytorchbot label "release notes: none"

@pytorch-bot pytorch-bot bot added the release notes: none Do not include this in the release notes label Jun 2, 2025
@tamird tamird force-pushed the export-D75806635 branch from 7d8f2f1 to 85b7457 Compare June 2, 2025 19:17
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75806635

@tamird tamird force-pushed the export-D75806635 branch 2 times, most recently from b5d5f83 to 2934fe0 Compare June 2, 2025 19:37
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75806635

@tamird tamird force-pushed the export-D75806635 branch 2 times, most recently from 3c4d8ff to 9f04bfc Compare June 2, 2025 22:04
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75806635

1 similar comment
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75806635

@tamird tamird force-pushed the export-D75806635 branch 2 times, most recently from 69f0ca3 to 95c9c56 Compare June 3, 2025 01:31
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75806635

@tamird tamird force-pushed the export-D75806635 branch 2 times, most recently from e96a68e to c0cce9c Compare June 3, 2025 09:54
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75806635

@tamird tamird force-pushed the export-D75806635 branch from c0cce9c to 183042c Compare June 3, 2025 09:59
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75806635

tamird pushed a commit to tamird/executorch that referenced this pull request Jun 3, 2025
@facebook-github-bot
Fix std::aligned_alloc feature detection. (pytorch#11293)
f7fcce4 
Summary:

The current feature detection seems to be incorrect, resulting in fallback to
the manual implementation, which in turn returns untagged pointers under
hwasan.

Replace the feature detection with a check on `__cplusplus`.

While I'm here, use smart pointers to manage memory where it is simple to do
so and remove allocation size rounding where it isn't required.

Differential Revision: D75806635
@tamird tamird force-pushed the export-D75806635 branch from 183042c to f7fcce4 Compare June 3, 2025 10:03
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75806635

tamird pushed a commit to tamird/executorch that referenced this pull request Jun 3, 2025
@facebook-github-bot
Fix std::aligned_alloc feature detection. (pytorch#11293)
3ab2dfc 
Summary:
Pull Request resolved: pytorch#11293

The current feature detection seems to be incorrect, resulting in fallback to
the manual implementation, which in turn returns untagged pointers under
hwasan.

Replace the feature detection with a check on `__cplusplus`.

While I'm here, use smart pointers to manage memory where it is simple to do
so and remove allocation size rounding where it isn't required.

Differential Revision: D75806635
tamird pushed a commit to tamird/executorch that referenced this pull request Jun 3, 2025
@facebook-github-bot
Use __cpp_aligned_new instead of hand-rolling the implementation (p…
93292fd 
…ytorch#11293)

Summary:
Pull Request resolved: pytorch#11293

The current feature detection seems to be incorrect, resulting in fallback to
the manual implementation, which in turn returns untagged pointers under
hwasan.

Use new expressions instead to delegate the work to the C++ compiler.

Reviewed By: lucylq

Differential Revision: D75806635
@tamird tamird force-pushed the export-D75806635 branch 2 times, most recently from 93292fd to 40d4b57 Compare June 4, 2025 00:56
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75806635

@tamird tamird force-pushed the export-D75806635 branch from 40d4b57 to 214fe03 Compare June 4, 2025 08:10
tamird pushed a commit to tamird/executorch that referenced this pull request Jun 4, 2025
@facebook-github-bot
Use __cpp_aligned_new instead of hand-rolling the implementation (p…
214fe03 
…ytorch#11293)

Summary:

The current feature detection seems to be incorrect, resulting in fallback to
the manual implementation, which in turn returns untagged pointers under
hwasan.

Use new expressions instead to delegate the work to the C++ compiler.

Reviewed By: lucylq

Differential Revision: D75806635
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75806635

tamird pushed a commit to tamird/executorch that referenced this pull request Jun 4, 2025
@facebook-github-bot
Use __cpp_aligned_new instead of hand-rolling the implementation (p…
dc01820 
…ytorch#11293)

Summary:
Pull Request resolved: pytorch#11293

The current feature detection seems to be incorrect, resulting in fallback to
the manual implementation, which in turn returns untagged pointers under
hwasan.

Use new expressions instead to delegate the work to the C++ compiler.

Reviewed By: lucylq

Differential Revision: D75806635
@tamird tamird force-pushed the export-D75806635 branch from 214fe03 to dc01820 Compare June 4, 2025 08:13
@tamird
Copy link
Contributor Author

tamird commented Jun 4, 2025

@swolchok this keeps failing to land with:

The following GitHub Pull Request(s) are not mergeable on GitHub: #11293. This Diff cannot land if the Pull Request cannot be merged. Please rebase the diff and re-export it to GitHub or resolve the merge conflict on GitHub and import the changes.

Yet this PR shows no merge conflicts. This has persisted through several rebases. Do you know what's going on?

@tamird
Copy link
Contributor Author

tamird commented Jun 4, 2025

The problem seems to be:

$ curl -sfSL https://api.github.com/repos/pytorch/executorch/pulls/11293 | jq .mergeable_state
"unstable"

which is an undocumented field. Why is that used to determine whether it is mergeable or not?

tamird pushed a commit to tamird/executorch that referenced this pull request Jun 4, 2025
@facebook-github-bot
Use __cpp_aligned_new instead of hand-rolling the implementation (p…
a20a998 
…ytorch#11293)

Summary:

The current feature detection seems to be incorrect, resulting in fallback to
the manual implementation, which in turn returns untagged pointers under
hwasan.

Use new expressions instead to delegate the work to the C++ compiler.

Reviewed By: lucylq

Differential Revision: D75806635
@tamird tamird force-pushed the export-D75806635 branch from dc01820 to a20a998 Compare June 4, 2025 14:39
tamird pushed a commit to tamird/executorch that referenced this pull request Jun 4, 2025
@facebook-github-bot
Use __cpp_aligned_new instead of hand-rolling the implementation (p…
8dec751 
…ytorch#11293)

Summary:

The current feature detection seems to be incorrect, resulting in fallback to
the manual implementation, which in turn returns untagged pointers under
hwasan.

Use new expressions instead to delegate the work to the C++ compiler.

Reviewed By: lucylq

Differential Revision: D75806635
@tamird tamird force-pushed the export-D75806635 branch from a20a998 to 8dec751 Compare June 4, 2025 14:39
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75806635

1 similar comment
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75806635

@tamird tamird force-pushed the export-D75806635 branch from 8dec751 to 71575dd Compare June 4, 2025 14:43
tamird pushed a commit to tamird/executorch that referenced this pull request Jun 4, 2025
@facebook-github-bot
Use __cpp_aligned_new instead of hand-rolling the implementation (p…
71575dd 
…ytorch#11293)

Summary:
Pull Request resolved: pytorch#11293

The current feature detection seems to be incorrect, resulting in fallback to
the manual implementation, which in turn returns untagged pointers under
hwasan.

Use new expressions instead to delegate the work to the C++ compiler.

Reviewed By: lucylq

Differential Revision: D75806635
@swolchok
Copy link
Contributor

swolchok commented Jun 4, 2025

@swolchok this keeps failing to land with:

The following GitHub Pull Request(s) are not mergeable on GitHub: #11293. This Diff cannot land if the Pull Request cannot be merged. Please rebase the diff and re-export it to GitHub or resolve the merge conflict on GitHub and import the changes.

Yet this PR shows no merge conflicts. This has persisted through several rebases. Do you know what's going on?

how are you updating the PR? if you sent it out internally, you need to update the internal diff and export that diff to github; I see force pushes which looks like maybe you are updating the PR through some separate mechanism like working with github directly.

@tamird tamird force-pushed the export-D75806635 branch from 71575dd to 760b33b Compare June 4, 2025 16:36
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75806635

@tamird tamird force-pushed the export-D75806635 branch from 760b33b to 00f2dce Compare June 4, 2025 16:44
@tamird
Copy link
Contributor Author

tamird commented Jun 4, 2025

@swolchok this keeps failing to land with:

The following GitHub Pull Request(s) are not mergeable on GitHub: #11293. This Diff cannot land if the Pull Request cannot be merged. Please rebase the diff and re-export it to GitHub or resolve the merge conflict on GitHub and import the changes.

Yet this PR shows no merge conflicts. This has persisted through several rebases. Do you know what's going on?

how are you updating the PR? if you sent it out internally, you need to update the internal diff and export that diff to github; I see force pushes which looks like maybe you are updating the PR through some separate mechanism like working with github directly.

No, those force pushes are done by phabricator.

It seems that PRs that have both internal and external changes are broken because the mergeable_state is always "unstable".

tamird pushed a commit to tamird/executorch that referenced this pull request Jun 4, 2025
@facebook-github-bot
Use __cpp_aligned_new instead of hand-rolling the implementation (p…
ffffd0f 
…ytorch#11293)

Summary:

The current feature detection seems to be incorrect, resulting in fallback to
the manual implementation, which in turn returns untagged pointers under
hwasan.

Use new expressions instead to delegate the work to the C++ compiler.

Reviewed By: lucylq

Differential Revision: D75806635
@tamird tamird force-pushed the export-D75806635 branch from 00f2dce to ffffd0f Compare June 4, 2025 18:44
@facebook-github-bot
Use __cpp_aligned_new instead of hand-rolling the implementation (p…
f7fbffe 
…ytorch#11293)

Summary:
Pull Request resolved: pytorch#11293

The current feature detection seems to be incorrect, resulting in fallback to
the manual implementation, which in turn returns untagged pointers under
hwasan.

Use new expressions instead to delegate the work to the C++ compiler.

Reviewed By: lucylq

Differential Revision: D75806635
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75806635

@tamird tamird force-pushed the export-D75806635 branch from ffffd0f to f7fbffe Compare June 4, 2025 18:47
@facebook-github-bot facebook-github-bot merged commit aa67f08 into pytorch:main Jun 4, 2025
169 of 171 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swolchok swolchok swolchok left review comments

@lucylq lucylq lucylq approved these changes

@JacobSzwejbka JacobSzwejbka Awaiting requested review from JacobSzwejbka JacobSzwejbka is a code owner

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported release notes: none Do not include this in the release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tamird @facebook-github-bot @swolchok @lucylq