[ATen-CPU] Add `math.h` for Gelu by SS-JIA · Pull Request #149164 · pytorch/pytorch

SS-JIA · 2025-03-13T23:02:38Z

Summary:

Context

This PR is mostly to enable ExecuTorch build for Windows: pytorch/executorch#9198

In ExecuTorch, the optimized GeLU kernel calls the ATen implementation. However, on Windows math.h needs to be included with #define _USE_MATH_DEFINES in order for math constants to be defined.

Test Plan:
Rely on CI to make sure existing tests do not break. Tested separately with ExecuTorch to make sure Windows build is successful.

cc @jgong5 @mingfeima @XiaobingSuper @sanchitintel @ashokei @jingxu10

pytorch-bot · 2025-03-13T23:02:42Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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

❌ 1 Cancelled Job

As of commit 7bc2c88 with merge base 71795f1 ():

CANCELLED JOB - The following job was cancelled. Please retry:

BC Lint (gh)

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

swolchok · 2025-03-13T23:10:49Z

+// access constants such as M_SQRT2 and M_2_SQRTPI.
+#ifdef _WIN32
+#define _USE_MATH_DEFINES
+#include <math.h>


why not include this header unconditionally?

also can we use instead?

I mainly wanted to limit the scope of the changes to Windows only builds. But I can see the case for making it unconditional, will update.

also can we use instead?

Sorry, I don't understand this comment 😅

I meant "can we use <cmath> instead"? I guess GitHub thought it was an HTML tag or something

SS-JIA · 2025-03-14T16:11:59Z

<cmath> seems to trigger lintrunner failures: https://github.com/pytorch/pytorch/actions/runs/13846450782/job/38745821504?pr=149164

Error message snippet:

>>> Lint for ../../usr/include/c++/11/cmath:

  Error (CLANGTIDY) [clang-diagnostic-error]
    constexpr function 'fpclassify' without __host__ or __device__ attributes
    cannot overload __device__ function with the same signature; add a
    __host__ attribute, or build with -fno-cuda-host-device-constexpr

         534  |
         535  |#ifndef __CORRECT_ISO_CPP11_MATH_H_PROTO_FP
         536  |  constexpr int
    >>>  537  |  fpclassify(float __x)
         538  |  { return __builtin_fpclassify(FP_NAN, FP_INFINITE, FP_NORMAL,
         539  |				FP_SUBNORMAL, FP_ZERO, __x); }
         540  |

  Error (CLANGTIDY) [clang-diagnostic-error]
    constexpr function 'fpclassify' without __host__ or __device__ attributes
    cannot overload __device__ function with the same signature; add a
    __host__ attribute, or build with -fno-cuda-host-device-constexpr

         539  |				FP_SUBNORMAL, FP_ZERO, __x); }
         540  |
         541  |  constexpr int
    >>>  542  |  fpclassify(double __x)
         543  |  { return __builtin_fpclassify(FP_NAN, FP_INFINITE, FP_NORMAL,
         544  |				FP_SUBNORMAL, FP_ZERO, __x); }
         545  |

...

Will just hide the include in the ifdef block for now...

Summary: ## Context This PR is mostly to enable ExecuTorch build for Windows: pytorch/executorch#9198 In ExecuTorch, the optimized GeLU kernel calls the ATen implementation. However, on Windows `math.h` needs to be included with `#define _USE_MATH_DEFINES` in order for math constants to be defined. Test Plan: Rely on CI to make sure existing tests do not break. Tested separately with ExecuTorch to make sure Windows build is successful.

SS-JIA · 2025-03-14T16:34:19Z

@pytorchbot merge

pytorchmergebot · 2025-03-14T16:35:56Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

pytorchmergebot · 2025-03-14T16:36:08Z

Merge failed

Reason: 1 mandatory check(s) failed. The first few are:

BC Lint

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

Failing merge rule: Core Maintainers

SS-JIA · 2025-03-14T20:04:11Z

@pytorchbot merge

pytorchmergebot · 2025-03-14T20:05:56Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

pytorchmergebot · 2025-03-14T20:06:06Z

Merge failed

Reason: 1 mandatory check(s) failed. The first few are:

BC Lint

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

Failing merge rule: Core Maintainers

SS-JIA · 2025-03-14T23:35:33Z

@pytorchbot merge -f "The bot is complaining about BC Lint, but the job keeps getting cancelled immediately. I don't think the signal is accurate. All other jobs are green."

pytorchmergebot · 2025-03-14T23:37:10Z

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

Summary: ## Context See #149164 for more context. Originally, this fix worked but more recently including `cmath` by itself no longer provides access to math constants on Windows platforms. I found that including `math.h` resolves this. I'm not sure exactly what changed, but this PR updates the header to just use both includes fix the symbols not being found. It might be a bug with a recent Windows update perhaps? Test Plan: CI

Summary: ## Context See #149164 for more context. Originally, this fix worked but more recently including `cmath` by itself no longer provides access to math constants on Windows platforms. I found that including `math.h` resolves this. I'm not sure exactly what changed, but this PR updates the header to just use both includes fix the symbols not being found. It might be a bug with a recent Windows update perhaps? Test Plan: CI Pull Request resolved: #153742 Approved by: https://github.com/swolchok, https://github.com/Skylion007

pytorch-bot Bot added the module: cpu CPU specific problem (e.g., perf, algorithm) label Mar 13, 2025

SS-JIA requested a review from swolchok March 13, 2025 23:02

SS-JIA mentioned this pull request Mar 13, 2025

[Windows] misc fixes to enable Windows build pytorch/executorch#9198

Merged

swolchok approved these changes Mar 13, 2025

View reviewed changes

SS-JIA force-pushed the pr149164 branch from 84e4569 to 032ca09 Compare March 13, 2025 23:18

SS-JIA force-pushed the pr149164 branch from 032ca09 to 7bc2c88 Compare March 14, 2025 16:18

SS-JIA added the topic: not user facing topic category label Mar 14, 2025

pytorch-bot Bot added the ciflow/trunk Trigger trunk jobs on your pull request label Mar 14, 2025

pytorchmergebot added the merging label Mar 14, 2025

pytorchmergebot removed the merging label Mar 14, 2025

pytorchmergebot added the merging label Mar 14, 2025

pytorchmergebot removed the merging label Mar 14, 2025

pytorchmergebot added the merging label Mar 14, 2025

pytorchmergebot added the Merged label Mar 14, 2025

pytorchmergebot closed this in ff58cce Mar 14, 2025

pytorchmergebot removed the merging label Mar 14, 2025

github-actions Bot deleted the pr149164 branch April 20, 2025 02:22

SS-JIA mentioned this pull request May 16, 2025

[ATen-CPU] Use math.h for GeLU as well as cmath #153742

Closed

Conversation

SS-JIA commented Mar 13, 2025 • edited by pytorch-bot Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Uh oh!

pytorch-bot Bot commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

❌ 1 Cancelled Job

Uh oh!

swolchok Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

SS-JIA Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swolchok Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

SS-JIA commented Mar 14, 2025

Uh oh!

SS-JIA commented Mar 14, 2025

Uh oh!

pytorchmergebot commented Mar 14, 2025

Merge started

Uh oh!

pytorchmergebot commented Mar 14, 2025

Merge failed

Uh oh!

SS-JIA commented Mar 14, 2025

Uh oh!

pytorchmergebot commented Mar 14, 2025

Merge started

Uh oh!

pytorchmergebot commented Mar 14, 2025

Merge failed

Uh oh!

SS-JIA commented Mar 14, 2025

Uh oh!

pytorchmergebot commented Mar 14, 2025

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SS-JIA commented Mar 13, 2025 •

edited by pytorch-bot Bot

Loading

pytorch-bot Bot commented Mar 13, 2025 •

edited

Loading

SS-JIA Mar 13, 2025 •

edited

Loading