Skip to content

Commit 76c4b48

Browse files
committed
Give targets that link torch_cpu_library torch's compile settings back
Linking the torch target does more than link. TorchConfig also gives it TORCH_INCLUDE_DIRS, which CMake treats as system headers because the target is imported, TORCH_CXX_FLAGS, and on recent torch a minimum C++ standard (C++20). torch_cpu_library, which our targets now link instead so that a CPU-only torch can load them, only passes on torch/include. So those targets changed. torch/csrc/api/include became a normal include directory, so warnings inside torch's headers are now reported as if they were our code. And a TORCH_CXX_FLAGS set at configure time no longer reached them. For example, compiling kernels/portable/cpu/op_allclose.cpp with -Wall -Wextra -Werror went from no errors to 25 errors, all inside torch's headers. Default builds were not affected. find_package_torch now copies torch's interface include directories, compile options and compile features to torch_cpu_library. It does this once, also when the project found torch before calling it. Every target that links torch_cpu_library, directly or through a PUBLIC link such as custom_ops_aot_lib, now gets these settings, and a new link site cannot forget them. With a CUDA build of PyTorch, torch also carries the CUDA include directory and definitions of torch_cuda_library. Those are not copied, since these targets do not use CUDA. A new test configures a small project and checks that a target linking torch_cpu_library, directly or through a PUBLIC link, gets torch's C++ API include directory, TORCH_CXX_FLAGS and C++ standard. It checks both when find_package_torch finds torch and when the project found it first. It passes with CPU and CUDA builds of torch and fails without this change.
1 parent e14d655 commit 76c4b48

2 files changed

Lines changed: 138 additions & 0 deletions

File tree

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
# Copyright (c) Meta Platforms, Inc. and affiliates.
2+
# All rights reserved.
3+
#
4+
# This source code is licensed under the BSD-style license found in the
5+
# LICENSE file in the root directory of this source tree.
6+
7+
import importlib.util
8+
import json
9+
import shlex
10+
import shutil
11+
import subprocess
12+
import sys
13+
import tempfile
14+
import unittest
15+
from pathlib import Path
16+
17+
ROOT = Path(__file__).resolve().parents[3]
18+
19+
20+
def _compile_flags(entry: dict) -> set:
21+
"""The flags of one compile command, without the compiler, source and outputs."""
22+
flags = set()
23+
arguments = iter(shlex.split(entry["command"])[1:])
24+
for argument in arguments:
25+
if argument in ("-o", "-c", "-MT", "-MF"):
26+
next(arguments)
27+
elif argument == "-isystem":
28+
flags.add(f"-isystem {next(arguments)}")
29+
elif argument != "-MD":
30+
flags.add(argument)
31+
return flags
32+
33+
34+
def _copied_from_torch(flag: str) -> bool:
35+
"""Whether a flag comes from what find_package_torch copies from torch.
36+
37+
A CUDA build of torch also gives torch the CUDA toolkit include dir and torch_cuda's
38+
definitions, which torch_cpu_library correctly does not carry.
39+
"""
40+
return (
41+
flag == "-DET_TORCH_CXX_FLAGS_PROBE"
42+
or flag.startswith("-std=")
43+
or (flag.startswith("-isystem ") and flag.endswith("torch/csrc/api/include"))
44+
)
45+
46+
47+
@unittest.skipIf(sys.platform == "win32", "Visual Studio writes no compile commands")
48+
class TestTorchCpuLibraryCMake(unittest.TestCase):
49+
def test_torch_cpu_library_gets_torch_settings(self):
50+
cmake = shutil.which("cmake")
51+
if cmake is None or importlib.util.find_spec("torch") is None:
52+
self.skipTest("CMake and torch are required")
53+
54+
found_first = (
55+
"add_torch_to_cmake_prefix_path()\nfind_package(Torch CONFIG REQUIRED)"
56+
)
57+
for find_torch in ("", found_first):
58+
with self.subTest(found_first=bool(find_torch)):
59+
self._check(cmake, find_torch)
60+
61+
def _check(self, cmake: str, find_torch: str) -> None:
62+
with tempfile.TemporaryDirectory() as directory:
63+
source = Path(directory)
64+
build = source / "build"
65+
(source / "probe.cpp").write_text("int probe() { return 0; }\n")
66+
(source / "CMakeLists.txt").write_text(
67+
f"""cmake_minimum_required(VERSION 3.24)
68+
project(TorchCpuLibraryProbe LANGUAGES CXX)
69+
include("${{EXECUTORCH_ROOT}}/tools/cmake/Utils.cmake")
70+
{find_torch}
71+
find_package_torch()
72+
add_library(links_torch OBJECT probe.cpp)
73+
target_link_libraries(links_torch PRIVATE torch)
74+
add_library(links_torch_cpu_library OBJECT probe.cpp)
75+
target_link_libraries(links_torch_cpu_library PRIVATE torch_cpu_library)
76+
add_library(links_it_publicly STATIC probe.cpp)
77+
target_link_libraries(links_it_publicly PUBLIC torch_cpu_library)
78+
add_library(consumer OBJECT probe.cpp)
79+
target_link_libraries(consumer PRIVATE links_it_publicly)
80+
"""
81+
)
82+
result = subprocess.run(
83+
[
84+
cmake,
85+
"-S",
86+
str(source),
87+
"-B",
88+
str(build),
89+
f"-DEXECUTORCH_ROOT={ROOT}",
90+
f"-DPYTHON_EXECUTABLE={sys.executable}",
91+
"-DCMAKE_EXPORT_COMPILE_COMMANDS=ON",
92+
# Recent torch sets no TORCH_CXX_FLAGS, so set one to check it travels too.
93+
"-DTORCH_CXX_FLAGS=-DET_TORCH_CXX_FLAGS_PROBE",
94+
],
95+
capture_output=True,
96+
text=True,
97+
)
98+
self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
99+
100+
flags = {}
101+
for entry in json.loads((build / "compile_commands.json").read_text()):
102+
target = Path(entry["output"]).parent.name.removesuffix(".dir")
103+
flags[target] = _compile_flags(entry)
104+
expected = {
105+
flag for flag in flags["links_torch"] if _copied_from_torch(flag)
106+
}
107+
self.assertIn("-DET_TORCH_CXX_FLAGS_PROBE", expected)
108+
self.assertTrue(any(flag.startswith("-isystem ") for flag in expected))
109+
for target in ("links_torch_cpu_library", "links_it_publicly", "consumer"):
110+
self.assertEqual(expected - flags[target], set(), target)
111+
112+
113+
if __name__ == "__main__":
114+
unittest.main()

‎tools/cmake/Utils.cmake‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,30 @@ macro(find_package_torch)
278278
add_torch_to_cmake_prefix_path()
279279
find_package(Torch CONFIG REQUIRED)
280280
endif()
281+
# TorchConfig gives its include dirs, compile flags and C++ standard to torch
282+
# only. Our targets link torch_cpu_library instead, so copy them there once,
283+
# also when the project found torch before calling this.
284+
get_target_property(
285+
_torch_settings_copied torch_cpu_library EXECUTORCH_TORCH_SETTINGS_COPIED
286+
)
287+
if(NOT _torch_settings_copied)
288+
foreach(_torch_property
289+
INTERFACE_INCLUDE_DIRECTORIES INTERFACE_COMPILE_OPTIONS
290+
INTERFACE_COMPILE_FEATURES
291+
)
292+
get_target_property(_torch_value torch ${_torch_property})
293+
if(_torch_value)
294+
set_property(
295+
TARGET torch_cpu_library
296+
APPEND
297+
PROPERTY ${_torch_property} ${_torch_value}
298+
)
299+
endif()
300+
endforeach()
301+
set_property(
302+
TARGET torch_cpu_library PROPERTY EXECUTORCH_TORCH_SETTINGS_COPIED TRUE
303+
)
304+
endif()
281305
endmacro()
282306

283307
# Modify ${targetName}'s INTERFACE_INCLUDE_DIRECTORIES by wrapping each entry in

0 commit comments

Comments
 (0)