bugfix: ensure that at::{dispatch_key}:: API gets external linkage by bdhirsh · Pull Request #58569 · pytorch/pytorch

bdhirsh · 2021-05-19T17:27:07Z

This should allow external C++ files that aren't compiled into libtorch.so/libtorch_cpu.so (including all of fbcode) to use fast path functions like at::cpu::add(), which skip the dispatcher.

So, after spending way too much time trying to figure out why I was getting linker errors when calling at::meta::{op} and at::cpu::{op} from C++ test files, I realized that we're not including the header files for C++ for the namespaced operator definitions. I.e. RegisterCPU.cpp, which provides definitions for the at::cpu::{op} fast path functions, wasn't including the CPUFunctions.h header.

Why that breaks stuff: the CPUFunctions.h header file is what marks each function with the TORCH_API macro, so without including it, when we build libtorch.so and libtorch_cpu.so, the compiler will look at the definition in RegisterCPU.cpp, not see a TORCH_API, and decide that the function should get internal linkage.

An alternative would be to directly mark the function definitions in RegisterCPU.cpp with TORCH_API, but this seemed cleaner.

Stack from ghstack:

move all external kernels into a class for better compiler error messages #59839 move all external kernels into a class for better compiler error messages
add a boxed CPU fallback kernel #58065 add a boxed CPU fallback kernel
beef up at::_ops API #59115 beef up at::_ops API
generate C++ API for meta functions using at::meta:: #58570 generate C++ API for meta functions using at::meta::
bugfix: ensure that at::{dispatch_key}:: API gets external linkage #58569 bugfix: ensure that at::{dispatch_key}:: API gets external linkage

Differential Revision: D28711300

[ghstack-poisoned]

facebook-github-bot · 2021-05-19T17:27:14Z

💊 CI failures summary and remediations

As of commit f95e411 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

bdhirsh · 2021-05-19T19:01:16Z

tools/codegen/gen_backend_stubs.py

                'extra_cuda_headers': '',
                'legacy_th_headers': '',
                'external_backend_headers': external_backend_headers,
+                'namespaced_headers': '',


I haven't actually started codegen'ing the XLAFunctions.h file that exposes the at::xla::{op} fast path API - when we eventually do that we'll need the same logic here.

… linkage" This should allow external C++ files that aren't compiled into `libtorch.so`/`libtorch_cpu.so` (including all of fbcode) to use the fast path `at::{namespace}::{op}` fast path functions that skip the dispatcher. So, after spending way too much time trying to figure out why I was getting linker errors when calling `at::meta::{op}` and `at::cpu::{op}` from C++ test files, I realized that we're not including the header files for C++ for the namespaced operator definitions. I.e. `RegisterCPU.cpp`, which provides definitions for the `at::cpu::{op}` fast path functions, wasn't including the `CPUFunctions.h` header. Why that breaks stuff: the `CPUFunctions.h` header file is what marks each function with the `TORCH_API` macro, so without including it, when we build `libtorch.so` and `libtorch_cpu.so`, the compiler will look at the definition in `RegisterCPU.cpp`, not see a `TORCH_API`, and decide that the function should get internal linkage. [ghstack-poisoned]

… linkage" This should allow external C++ files that aren't compiled into `libtorch.so`/`libtorch_cpu.so` (including all of fbcode) to use fast path functions like `at::cpu::add()`, which skip the dispatcher. So, after spending way too much time trying to figure out why I was getting linker errors when calling `at::meta::{op}` and `at::cpu::{op}` from C++ test files, I realized that we're not including the header files for C++ for the namespaced operator definitions. I.e. `RegisterCPU.cpp`, which provides definitions for the `at::cpu::{op}` fast path functions, wasn't including the `CPUFunctions.h` header. Why that breaks stuff: the `CPUFunctions.h` header file is what marks each function with the `TORCH_API` macro, so without including it, when we build `libtorch.so` and `libtorch_cpu.so`, the compiler will look at the definition in `RegisterCPU.cpp`, not see a `TORCH_API`, and decide that the function should get internal linkage. An alternative would be to directly mark the function definitions in `RegisterCPU.cpp` with `TORCH_API`, but this seemed cleaner. [ghstack-poisoned]

ezyang

Nice catch, sorry about sending you on a wild bugfix hunt.

… linkage" This should allow external C++ files that aren't compiled into `libtorch.so`/`libtorch_cpu.so` (including all of fbcode) to use fast path functions like `at::cpu::add()`, which skip the dispatcher. So, after spending way too much time trying to figure out why I was getting linker errors when calling `at::meta::{op}` and `at::cpu::{op}` from C++ test files, I realized that we're not including the header files for C++ for the namespaced operator definitions. I.e. `RegisterCPU.cpp`, which provides definitions for the `at::cpu::{op}` fast path functions, wasn't including the `CPUFunctions.h` header. Why that breaks stuff: the `CPUFunctions.h` header file is what marks each function with the `TORCH_API` macro, so without including it, when we build `libtorch.so` and `libtorch_cpu.so`, the compiler will look at the definition in `RegisterCPU.cpp`, not see a `TORCH_API`, and decide that the function should get internal linkage. An alternative would be to directly mark the function definitions in `RegisterCPU.cpp` with `TORCH_API`, but this seemed cleaner. [ghstack-poisoned]

bdhirsh · 2021-05-26T15:24:08Z

@bdhirsh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ezyang · 2021-05-27T18:09:25Z

need this for one of my PRs

… linkage" This should allow external C++ files that aren't compiled into `libtorch.so`/`libtorch_cpu.so` (including all of fbcode) to use fast path functions like `at::cpu::add()`, which skip the dispatcher. So, after spending way too much time trying to figure out why I was getting linker errors when calling `at::meta::{op}` and `at::cpu::{op}` from C++ test files, I realized that we're not including the header files for C++ for the namespaced operator definitions. I.e. `RegisterCPU.cpp`, which provides definitions for the `at::cpu::{op}` fast path functions, wasn't including the `CPUFunctions.h` header. Why that breaks stuff: the `CPUFunctions.h` header file is what marks each function with the `TORCH_API` macro, so without including it, when we build `libtorch.so` and `libtorch_cpu.so`, the compiler will look at the definition in `RegisterCPU.cpp`, not see a `TORCH_API`, and decide that the function should get internal linkage. An alternative would be to directly mark the function definitions in `RegisterCPU.cpp` with `TORCH_API`, but this seemed cleaner. Differential Revision: [D28711300](https://our.internmc.facebook.com/intern/diff/D28711300) [ghstack-poisoned]

bdhirsh · 2021-06-02T13:11:38Z

@bdhirsh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

… linkage" This should allow external C++ files that aren't compiled into `libtorch.so`/`libtorch_cpu.so` (including all of fbcode) to use fast path functions like `at::cpu::add()`, which skip the dispatcher. So, after spending way too much time trying to figure out why I was getting linker errors when calling `at::meta::{op}` and `at::cpu::{op}` from C++ test files, I realized that we're not including the header files for C++ for the namespaced operator definitions. I.e. `RegisterCPU.cpp`, which provides definitions for the `at::cpu::{op}` fast path functions, wasn't including the `CPUFunctions.h` header. Why that breaks stuff: the `CPUFunctions.h` header file is what marks each function with the `TORCH_API` macro, so without including it, when we build `libtorch.so` and `libtorch_cpu.so`, the compiler will look at the definition in `RegisterCPU.cpp`, not see a `TORCH_API`, and decide that the function should get internal linkage. An alternative would be to directly mark the function definitions in `RegisterCPU.cpp` with `TORCH_API`, but this seemed cleaner. Differential Revision: [D28711300](https://our.internmc.facebook.com/intern/diff/D28711300) [ghstack-poisoned]

bdhirsh · 2021-06-02T14:05:40Z

@bdhirsh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

… linkage" This should allow external C++ files that aren't compiled into `libtorch.so`/`libtorch_cpu.so` (including all of fbcode) to use fast path functions like `at::cpu::add()`, which skip the dispatcher. So, after spending way too much time trying to figure out why I was getting linker errors when calling `at::meta::{op}` and `at::cpu::{op}` from C++ test files, I realized that we're not including the header files for C++ for the namespaced operator definitions. I.e. `RegisterCPU.cpp`, which provides definitions for the `at::cpu::{op}` fast path functions, wasn't including the `CPUFunctions.h` header. Why that breaks stuff: the `CPUFunctions.h` header file is what marks each function with the `TORCH_API` macro, so without including it, when we build `libtorch.so` and `libtorch_cpu.so`, the compiler will look at the definition in `RegisterCPU.cpp`, not see a `TORCH_API`, and decide that the function should get internal linkage. An alternative would be to directly mark the function definitions in `RegisterCPU.cpp` with `TORCH_API`, but this seemed cleaner. Differential Revision: [D28711300](https://our.internmc.facebook.com/intern/diff/D28711300) [ghstack-poisoned]

bdhirsh · 2021-06-10T23:34:20Z

@bdhirsh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

… linkage" This should allow external C++ files that aren't compiled into `libtorch.so`/`libtorch_cpu.so` (including all of fbcode) to use fast path functions like `at::cpu::add()`, which skip the dispatcher. So, after spending way too much time trying to figure out why I was getting linker errors when calling `at::meta::{op}` and `at::cpu::{op}` from C++ test files, I realized that we're not including the header files for C++ for the namespaced operator definitions. I.e. `RegisterCPU.cpp`, which provides definitions for the `at::cpu::{op}` fast path functions, wasn't including the `CPUFunctions.h` header. Why that breaks stuff: the `CPUFunctions.h` header file is what marks each function with the `TORCH_API` macro, so without including it, when we build `libtorch.so` and `libtorch_cpu.so`, the compiler will look at the definition in `RegisterCPU.cpp`, not see a `TORCH_API`, and decide that the function should get internal linkage. An alternative would be to directly mark the function definitions in `RegisterCPU.cpp` with `TORCH_API`, but this seemed cleaner. Differential Revision: [D28711300](https://our.internmc.facebook.com/intern/diff/D28711300) [ghstack-poisoned]

bdhirsh · 2021-06-11T00:51:05Z

@bdhirsh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

… linkage" This should allow external C++ files that aren't compiled into `libtorch.so`/`libtorch_cpu.so` (including all of fbcode) to use fast path functions like `at::cpu::add()`, which skip the dispatcher. So, after spending way too much time trying to figure out why I was getting linker errors when calling `at::meta::{op}` and `at::cpu::{op}` from C++ test files, I realized that we're not including the header files for C++ for the namespaced operator definitions. I.e. `RegisterCPU.cpp`, which provides definitions for the `at::cpu::{op}` fast path functions, wasn't including the `CPUFunctions.h` header. Why that breaks stuff: the `CPUFunctions.h` header file is what marks each function with the `TORCH_API` macro, so without including it, when we build `libtorch.so` and `libtorch_cpu.so`, the compiler will look at the definition in `RegisterCPU.cpp`, not see a `TORCH_API`, and decide that the function should get internal linkage. An alternative would be to directly mark the function definitions in `RegisterCPU.cpp` with `TORCH_API`, but this seemed cleaner. Differential Revision: [D28711300](https://our.internmc.facebook.com/intern/diff/D28711300) [ghstack-poisoned]

codecov · 2021-06-11T10:29:20Z

Codecov Report

Merging #58569 (1d9df16) into gh/bdhirsh/120/base (539a2fc) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 1d9df16 differs from pull request most recent head 87f095b. Consider uploading reports for the commit 87f095b to get more accurate results

@@                 Coverage Diff                  @@
##           gh/bdhirsh/120/base   #58569   +/-   ##
====================================================
  Coverage                76.27%   76.28%           
====================================================
  Files                     2041     2041           
  Lines                   203415   203415           
====================================================
+ Hits                    155152   155165   +13     
+ Misses                   48263    48250   -13

… linkage" This should allow external C++ files that aren't compiled into `libtorch.so`/`libtorch_cpu.so` (including all of fbcode) to use fast path functions like `at::cpu::add()`, which skip the dispatcher. So, after spending way too much time trying to figure out why I was getting linker errors when calling `at::meta::{op}` and `at::cpu::{op}` from C++ test files, I realized that we're not including the header files for C++ for the namespaced operator definitions. I.e. `RegisterCPU.cpp`, which provides definitions for the `at::cpu::{op}` fast path functions, wasn't including the `CPUFunctions.h` header. Why that breaks stuff: the `CPUFunctions.h` header file is what marks each function with the `TORCH_API` macro, so without including it, when we build `libtorch.so` and `libtorch_cpu.so`, the compiler will look at the definition in `RegisterCPU.cpp`, not see a `TORCH_API`, and decide that the function should get internal linkage. An alternative would be to directly mark the function definitions in `RegisterCPU.cpp` with `TORCH_API`, but this seemed cleaner. Differential Revision: [D28711300](https://our.internmc.facebook.com/intern/diff/D28711300) [ghstack-poisoned]

bdhirsh · 2021-06-11T15:14:32Z

@bdhirsh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

… linkage" This should allow external C++ files that aren't compiled into `libtorch.so`/`libtorch_cpu.so` (including all of fbcode) to use fast path functions like `at::cpu::add()`, which skip the dispatcher. So, after spending way too much time trying to figure out why I was getting linker errors when calling `at::meta::{op}` and `at::cpu::{op}` from C++ test files, I realized that we're not including the header files for C++ for the namespaced operator definitions. I.e. `RegisterCPU.cpp`, which provides definitions for the `at::cpu::{op}` fast path functions, wasn't including the `CPUFunctions.h` header. Why that breaks stuff: the `CPUFunctions.h` header file is what marks each function with the `TORCH_API` macro, so without including it, when we build `libtorch.so` and `libtorch_cpu.so`, the compiler will look at the definition in `RegisterCPU.cpp`, not see a `TORCH_API`, and decide that the function should get internal linkage. An alternative would be to directly mark the function definitions in `RegisterCPU.cpp` with `TORCH_API`, but this seemed cleaner. Differential Revision: [D28711300](https://our.internmc.facebook.com/intern/diff/D28711300) [ghstack-poisoned]

bdhirsh · 2021-06-11T21:43:24Z

@bdhirsh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

… linkage" This should allow external C++ files that aren't compiled into `libtorch.so`/`libtorch_cpu.so` (including all of fbcode) to use fast path functions like `at::cpu::add()`, which skip the dispatcher. So, after spending way too much time trying to figure out why I was getting linker errors when calling `at::meta::{op}` and `at::cpu::{op}` from C++ test files, I realized that we're not including the header files for C++ for the namespaced operator definitions. I.e. `RegisterCPU.cpp`, which provides definitions for the `at::cpu::{op}` fast path functions, wasn't including the `CPUFunctions.h` header. Why that breaks stuff: the `CPUFunctions.h` header file is what marks each function with the `TORCH_API` macro, so without including it, when we build `libtorch.so` and `libtorch_cpu.so`, the compiler will look at the definition in `RegisterCPU.cpp`, not see a `TORCH_API`, and decide that the function should get internal linkage. An alternative would be to directly mark the function definitions in `RegisterCPU.cpp` with `TORCH_API`, but this seemed cleaner. Differential Revision: [D28711300](https://our.internmc.facebook.com/intern/diff/D28711300) [ghstack-poisoned]

bdhirsh · 2021-06-14T14:08:10Z

@bdhirsh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

… linkage" This should allow external C++ files that aren't compiled into `libtorch.so`/`libtorch_cpu.so` (including all of fbcode) to use fast path functions like `at::cpu::add()`, which skip the dispatcher. So, after spending way too much time trying to figure out why I was getting linker errors when calling `at::meta::{op}` and `at::cpu::{op}` from C++ test files, I realized that we're not including the header files for C++ for the namespaced operator definitions. I.e. `RegisterCPU.cpp`, which provides definitions for the `at::cpu::{op}` fast path functions, wasn't including the `CPUFunctions.h` header. Why that breaks stuff: the `CPUFunctions.h` header file is what marks each function with the `TORCH_API` macro, so without including it, when we build `libtorch.so` and `libtorch_cpu.so`, the compiler will look at the definition in `RegisterCPU.cpp`, not see a `TORCH_API`, and decide that the function should get internal linkage. An alternative would be to directly mark the function definitions in `RegisterCPU.cpp` with `TORCH_API`, but this seemed cleaner. Differential Revision: [D28711300](https://our.internmc.facebook.com/intern/diff/D28711300) [ghstack-poisoned]

bdhirsh · 2021-06-14T19:37:30Z

@bdhirsh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

… linkage" This should allow external C++ files that aren't compiled into `libtorch.so`/`libtorch_cpu.so` (including all of fbcode) to use fast path functions like `at::cpu::add()`, which skip the dispatcher. So, after spending way too much time trying to figure out why I was getting linker errors when calling `at::meta::{op}` and `at::cpu::{op}` from C++ test files, I realized that we're not including the header files for C++ for the namespaced operator definitions. I.e. `RegisterCPU.cpp`, which provides definitions for the `at::cpu::{op}` fast path functions, wasn't including the `CPUFunctions.h` header. Why that breaks stuff: the `CPUFunctions.h` header file is what marks each function with the `TORCH_API` macro, so without including it, when we build `libtorch.so` and `libtorch_cpu.so`, the compiler will look at the definition in `RegisterCPU.cpp`, not see a `TORCH_API`, and decide that the function should get internal linkage. An alternative would be to directly mark the function definitions in `RegisterCPU.cpp` with `TORCH_API`, but this seemed cleaner. Differential Revision: [D28711300](https://our.internmc.facebook.com/intern/diff/D28711300) [ghstack-poisoned]

facebook-github-bot · 2021-06-15T23:55:08Z

@bdhirsh merged this pull request in e341bab.

bugfix: ensure that at::{dispatch_key}:: API gets external linkage

b4c7c25

[ghstack-poisoned]

facebook-github-bot added the cla signed label May 19, 2021

bdhirsh requested review from bhosmer and ezyang May 19, 2021 18:59

bdhirsh commented May 19, 2021

View reviewed changes

bdhirsh added 4 commits May 19, 2021 12:03

ezyang approved these changes May 20, 2021

View reviewed changes

ezyang mentioned this pull request May 27, 2021

Port silu_backward to structured #58661

Closed

bdhirsh mentioned this pull request May 27, 2021

beef up at::_ops API #59115

Closed

bdhirsh added 5 commits June 2, 2021 07:13

bdhirsh mentioned this pull request Jun 10, 2021

move all external kernels into a class for better compiler error messages #59839

Closed

bdhirsh added 2 commits June 15, 2021 10:00

facebook-github-bot closed this in e341bab Jun 15, 2021

facebook-github-bot added the Merged label Jun 15, 2021

facebook-github-bot deleted the gh/bdhirsh/120/head branch June 19, 2021 14:17

Conversation

bdhirsh commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

bdhirsh May 19, 2021

Choose a reason for hiding this comment

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

bdhirsh commented May 26, 2021

Uh oh!

ezyang commented May 27, 2021

Uh oh!

bdhirsh commented Jun 2, 2021

Uh oh!

bdhirsh commented Jun 2, 2021

Uh oh!

bdhirsh commented Jun 10, 2021

Uh oh!

bdhirsh commented Jun 11, 2021

Uh oh!

codecov bot commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bdhirsh commented Jun 11, 2021

Uh oh!

bdhirsh commented Jun 11, 2021

Uh oh!

bdhirsh commented Jun 14, 2021

Uh oh!

bdhirsh commented Jun 14, 2021

Uh oh!

facebook-github-bot commented Jun 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bdhirsh commented May 19, 2021 •

edited

Loading

facebook-github-bot commented May 19, 2021 •

edited

Loading

codecov bot commented Jun 11, 2021 •

edited

Loading