Skip to content

Commit 6a36ebe

Browse files
authored
Fix the ATen mode build of ETDump (pytorch#22135)
### Summary ETDump maps the device a tensor lives on to the runtime device type so it can pick the right allocator. In ATen mode it did that by calling `torch_to_executorch_device` from `extension/aten_util/aten_bridge.h`. That header cannot be included from a file compiled with `USE_ATEN_LIB`. It declares functions over `torch::executor::ScalarType` and `torch::executor::Tensor`, and those two names only exist when `runtime/core/exec_aten/exec_aten.h` takes its non ATen branch. So the include fired in exactly the one mode where it does not compile, and the ATen flavor of the ETDump library failed to build with 7 errors like: ``` no type named 'ScalarType' in namespace 'torch::executor' ``` Nothing in the CMake build compiles these files in ATen mode, so this is invisible in the CMake based CI jobs and only shows up for the Buck targets that carry the `_aten` suffix. ### What this changes * New header `extension/aten_util/aten_device.h` holds the two device conversions. They are small and self contained, so they are now `inline` functions that need only `c10/core/Device.h` and the portable `Device` type, and they compile in both modes. * `aten_bridge.h` includes the new header and keeps exporting the same names, so its callers do not change. It also gains a short note saying it is portable mode only, so the same include does not come back later. * `etdump_flatcc.cpp` includes the new header instead of the whole portable mode bridge, and the ATen only Buck dep points at the new target. * The portable `Device` type gets its own Buck target, so a caller that wants only `Device` does not have to depend on every portable type. This mirrors the existing `:scalar_type` split in the same file. * The new header is wrapped in `C10_DIAGNOSTIC_PUSH_AND_IGNORED_IF_DEFINED("-Wswitch-enum")`. `c10::DeviceType` has over twenty values, and `-Wswitch-enum` asks for every one to be listed even when the switch already has a default. `extension/tensor/tensor_ptr.h` already does the same thing for its own switch over a c10 enum. Plain `-Wswitch` is left alone, so an unhandled new ExecuTorch device type is still a compile error. No behavior change. The mapping itself is byte for byte the same code, just moved. ### Test plan Compiled with `-Wall -Wextra -Wswitch-enum -Werror` against PyTorch headers on Linux x86_64: | what | ATen mode | portable mode | | --- | --- | --- | | `aten_device.h` | clean | clean | | the ETDump device helper | clean | clean | | `aten_bridge.cpp` | not applicable | clean | | the call shapes the existing `aten_bridge` test uses | not applicable | clean | Each check was confirmed to fail when its fix is removed, so none of them is a test that cannot fail: * Without the move, the ATen mode compile of the ETDump helper fails with the 7 errors above. * Without the `-Wswitch-enum` guard, `aten_device.h` fails with 20 errors. * Adding a third value to the portable `DeviceType` enum still fails the build through plain `-Wswitch`, so the guard does not hide a missing case. What is not covered: no job in this repository builds the `_aten` Buck targets, so CI on this pull request exercises the portable build only. The ATen mode results above come from compiling those translation units by hand.
1 parent 6b53322 commit 6a36ebe

7 files changed

Lines changed: 110 additions & 48 deletions

File tree

‎devtools/etdump/etdump_flatcc.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
#include <executorch/runtime/platform/assert.h>
2323

2424
#ifdef USE_ATEN_LIB
25-
#include <executorch/extension/aten_util/aten_bridge.h>
25+
#include <executorch/extension/aten_util/aten_device.h>
2626
#endif
2727

2828
#include <flatcc/flatcc_types.h>

‎devtools/etdump/targets.bzl‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ def define_common_targets():
135135
"//executorch/runtime/core:device_allocator",
136136
"//executorch/runtime/platform:platform",
137137
] + ([
138-
"//executorch/extension/aten_util:aten_bridge",
138+
"//executorch/extension/aten_util:aten_device",
139139
] if aten_mode else []),
140140
exported_deps = [
141141
":etdump_schema_flatcc",

‎extension/aten_util/aten_bridge.cpp‎

Lines changed: 0 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -134,36 +134,6 @@ c10::ScalarType executorch_to_torch_scalar_type(
134134
return static_cast<c10::ScalarType>(intermediate);
135135
}
136136

137-
c10::Device executorch_to_torch_device(
138-
executorch::runtime::etensor::Device device) {
139-
switch (device.type()) {
140-
case executorch::runtime::etensor::DeviceType::CPU:
141-
return c10::Device(c10::DeviceType::CPU);
142-
case executorch::runtime::etensor::DeviceType::CUDA:
143-
return c10::Device(c10::DeviceType::CUDA, device.index());
144-
}
145-
// Aborting on an unknown device rather than falling back to CPU: a wrong
146-
// label made the host read accelerator memory and segfault.
147-
ET_CHECK_MSG(
148-
false,
149-
"Tensor reports device type %d, which this build cannot map to a PyTorch device",
150-
static_cast<int>(device.type()));
151-
}
152-
153-
std::optional<executorch::runtime::etensor::Device> torch_to_executorch_device(
154-
c10::Device device) {
155-
switch (device.type()) {
156-
case c10::DeviceType::CPU:
157-
return executorch::runtime::etensor::Device(
158-
executorch::runtime::etensor::DeviceType::CPU);
159-
case c10::DeviceType::CUDA:
160-
return executorch::runtime::etensor::Device(
161-
executorch::runtime::etensor::DeviceType::CUDA, device.index());
162-
default:
163-
return std::nullopt;
164-
}
165-
}
166-
167137
/*
168138
* Following makes two assumptions:
169139
* 1. aten_tensor's lifetime is longer than the liftime within which mutable_et

‎extension/aten_util/aten_bridge.h‎

Lines changed: 5 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,20 @@
88

99
#pragma once
1010

11+
// Portable mode only: the declarations below name torch::executor::ScalarType
12+
// and torch::executor::Tensor, which exec_aten.h defines only when it is not
13+
// building against ATen. For device conversions either way, use aten_device.h.
14+
15+
#include <executorch/extension/aten_util/aten_device.h>
1116
#include <executorch/extension/tensor/tensor.h>
1217
#include <executorch/runtime/core/exec_aten/exec_aten.h>
13-
#include <executorch/runtime/core/portable_type/device.h>
1418

1519
#include <ATen/Functions.h> // @manual=//caffe2/aten:ATen-cpu
1620
#include <ATen/Tensor.h> // @manual=//caffe2/aten:ATen-core
1721
#include <ATen/core/functional.h> // @manual=//caffe2/aten:ATen-core
1822
#include <c10/core/ScalarTypeToTypeMeta.h> // @manual=//caffe2/c10:c10
1923

2024
#include <memory>
21-
#include <optional>
2225
#include <vector>
2326

2427
namespace executorch {
@@ -30,19 +33,6 @@ torch::executor::ScalarType torch_to_executorch_scalar_type(
3033
c10::ScalarType executorch_to_torch_scalar_type(
3134
torch::executor::ScalarType type);
3235

33-
c10::Device executorch_to_torch_device(
34-
executorch::runtime::etensor::Device device);
35-
36-
/**
37-
* Maps a PyTorch device onto the ExecuTorch device naming the same location.
38-
*
39-
* Returns nothing for a device this runtime has no type for. That is a valid
40-
* thing for a caller to ask, so it is reported rather than fatal, and the
41-
* caller adds the context it has before failing.
42-
*/
43-
std::optional<executorch::runtime::etensor::Device> torch_to_executorch_device(
44-
c10::Device device);
45-
4636
/*
4737
* @param[in] aten_tensor Input at::Tensor
4838
* @param[in,out] mutable_et ETensor whose underlying memory now will alias to

‎extension/aten_util/aten_device.h‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
* All rights reserved.
4+
*
5+
* This source code is licensed under the BSD-style license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
#pragma once
10+
11+
#include <executorch/runtime/core/portable_type/device.h>
12+
#include <executorch/runtime/platform/assert.h>
13+
14+
#include <c10/core/Device.h> // @manual=//caffe2/c10:c10
15+
#include <c10/macros/Macros.h> // @manual=//caffe2/c10:c10
16+
17+
#include <optional>
18+
19+
// Kept out of aten_bridge.h, which compiles in portable mode only, so that a
20+
// translation unit built with USE_ATEN_LIB can still convert devices.
21+
22+
// c10::DeviceType has over twenty values and -Wswitch-enum wants every one
23+
// listed even with a default. -Wswitch still covers the switch below.
24+
C10_DIAGNOSTIC_PUSH_AND_IGNORED_IF_DEFINED("-Wswitch-enum")
25+
26+
namespace executorch {
27+
namespace extension {
28+
29+
/**
30+
* Maps an ExecuTorch device onto the PyTorch device naming the same location.
31+
*/
32+
inline c10::Device executorch_to_torch_device(
33+
executorch::runtime::etensor::Device device) {
34+
switch (device.type()) {
35+
case executorch::runtime::etensor::DeviceType::CPU:
36+
return c10::Device(c10::DeviceType::CPU);
37+
case executorch::runtime::etensor::DeviceType::CUDA:
38+
return c10::Device(c10::DeviceType::CUDA, device.index());
39+
}
40+
// Aborting on an unknown device rather than falling back to CPU: a wrong
41+
// label made the host read accelerator memory and segfault.
42+
ET_CHECK_MSG(
43+
false,
44+
"Tensor reports device type %d, which this build cannot map to a PyTorch device",
45+
static_cast<int>(device.type()));
46+
}
47+
48+
/**
49+
* Maps a PyTorch device onto the ExecuTorch device naming the same location.
50+
*
51+
* Returns nothing for a device this runtime has no type for. That is a valid
52+
* thing for a caller to ask, so it is reported rather than fatal, and the
53+
* caller adds the context it has before failing.
54+
*/
55+
inline std::optional<executorch::runtime::etensor::Device>
56+
torch_to_executorch_device(c10::Device device) {
57+
switch (device.type()) {
58+
case c10::DeviceType::CPU:
59+
return executorch::runtime::etensor::Device(
60+
executorch::runtime::etensor::DeviceType::CPU);
61+
case c10::DeviceType::CUDA:
62+
return executorch::runtime::etensor::Device(
63+
executorch::runtime::etensor::DeviceType::CUDA, device.index());
64+
default:
65+
return std::nullopt;
66+
}
67+
}
68+
69+
} // namespace extension
70+
} // namespace executorch
71+
72+
C10_DIAGNOSTIC_POP()

‎extension/aten_util/targets.bzl‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,21 @@ def define_common_targets():
77
TARGETS and BUCK files that call this function.
88
"""
99

10+
# Split out of :aten_bridge, which compiles in portable mode only, so that
11+
# ATen mode code can still convert devices.
12+
runtime.cxx_library(
13+
name = "aten_device",
14+
exported_headers = ["aten_device.h"],
15+
visibility = ["PUBLIC"],
16+
exported_deps = [
17+
"//executorch/runtime/core/portable_type:device",
18+
"//executorch/runtime/platform:platform",
19+
],
20+
exported_external_deps = [
21+
"libtorch",
22+
],
23+
)
24+
1025
runtime.cxx_library(
1126
name = "aten_bridge",
1227
srcs = ["aten_bridge.cpp"],
@@ -23,6 +38,7 @@ def define_common_targets():
2338
],
2439
visibility = ["PUBLIC"],
2540
exported_deps = [
41+
":aten_device",
2642
"//executorch/extension/kernel_util:kernel_util",
2743
"//executorch/extension/tensor:tensor",
2844
"//executorch/runtime/core:core",

‎runtime/core/portable_type/targets.bzl‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ def define_common_targets():
1919
"tensor.h",
2020
"tensor_impl.h",
2121
"string_view.h",
22-
"device.h",
2322
],
2423
# Only should be depended on by kernel_types:kernel_types, but various suffixes like Android and Static
2524
# mean I cant just expose visibility to a single rule.
@@ -36,6 +35,7 @@ def define_common_targets():
3635
"//executorch/runtime/core/portable_type/c10/c10:c10",
3736
],
3837
exported_deps = [
38+
":device",
3939
":scalar_type",
4040
"//executorch/runtime/core:core",
4141
"//executorch/runtime/core:tensor_shape_dynamism",
@@ -46,6 +46,20 @@ def define_common_targets():
4646
],
4747
)
4848

49+
# device.h is standalone and is the one portable type header that ATen mode
50+
# code also needs, so it is split out like :scalar_type below rather than
51+
# making a caller that wants only Device depend on every portable type.
52+
runtime.cxx_library(
53+
name = "device",
54+
exported_headers = [
55+
"device.h",
56+
],
57+
visibility = [
58+
"//executorch/extension/aten_util/...",
59+
"//executorch/runtime/core/...",
60+
],
61+
)
62+
4963
# Set up a specific exported library for scalar_type to avoid circle dependency in ScalarTypeUtil.h
5064
runtime.cxx_library(
5165
name = "scalar_type",

0 commit comments

Comments
 (0)