Skip to content

Commit a80d833

Browse files
committed
Guard aten_device.h against -Wswitch-enum
The new header switches over c10::DeviceType to map a PyTorch device onto an ExecuTorch one. That enum has over twenty values, and a build with -Wswitch-enum turned on asks for every one of them to be listed even though the switch already has a default. So the header does not compile there. This was hidden before. The same code used to live in aten_bridge.cpp, and the aten_bridge library is built with -Wno-error, which demotes the warning. Moving the code into a header exposed it to ETDump, which has no such setting. The fix wraps the file in C10_DIAGNOSTIC_PUSH_AND_IGNORED_IF_DEFINED("-Wswitch-enum"), the same way extension/tensor/tensor_ptr.h already does for its own switch over a c10 enum. Plain -Wswitch is left alone, so forgetting to handle a new ExecuTorch device type is still a compile error. Three smaller corrections to the previous commit come with it: - The comment on the new Buck target was wrong. Splitting device.h out does not keep the portable Tensor out of an ATen mode target, because the ETDump target already reaches it through runtime/core:device_allocator. The split is still worth doing, so that a caller wanting only Device does not depend on every portable type. That is the same reason :scalar_type is split out, and the comment now says so. - aten_bridge.h no longer uses std::optional, so its <optional> include is removed. - The new comments are shortened. Test plan: compiled aten_device.h and the ETDump device helper with and without USE_ATEN_LIB, under -Wall -Wextra -Wswitch-enum -Werror, against PyTorch headers on Linux x86_64. All four are clean. Removing only the new guard puts the 20 -Wswitch-enum errors back, so the guard is doing the work. Adding a third ExecuTorch device type still fails the build through plain -Wswitch, so the guard does not hide that. Also recompiled aten_bridge.cpp and the exact call shapes the existing aten_bridge test uses, in portable mode.
1 parent f47a717 commit a80d833

4 files changed

Lines changed: 16 additions & 13 deletions

File tree

extension/aten_util/aten_bridge.h

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,9 @@
88

99
#pragma once
1010

11-
// Portable mode only. The declarations below name torch::executor::ScalarType
11+
// Portable mode only: the declarations below name torch::executor::ScalarType
1212
// and torch::executor::Tensor, which exec_aten.h defines only when it is not
13-
// building against ATen. For the device conversions, which work either way,
14-
// include aten_device.h instead.
13+
// building against ATen. For device conversions either way, use aten_device.h.
1514

1615
#include <executorch/extension/aten_util/aten_device.h>
1716
#include <executorch/extension/tensor/tensor.h>
@@ -23,7 +22,6 @@
2322
#include <c10/core/ScalarTypeToTypeMeta.h> // @manual=//caffe2/c10:c10
2423

2524
#include <memory>
26-
#include <optional>
2725
#include <vector>
2826

2927
namespace executorch {

extension/aten_util/aten_device.h

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,16 @@
1212
#include <executorch/runtime/platform/assert.h>
1313

1414
#include <c10/core/Device.h> // @manual=//caffe2/c10:c10
15+
#include <c10/macros/Macros.h> // @manual=//caffe2/c10:c10
1516

1617
#include <optional>
1718

18-
// These live apart from aten_bridge.h so that a translation unit compiled with
19-
// USE_ATEN_LIB can convert devices. aten_bridge.h is usable only in portable
20-
// mode: it names torch::executor::ScalarType and torch::executor::Tensor,
21-
// aliases that exec_aten.h defines only when it is NOT building against ATen.
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")
2225

2326
namespace executorch {
2427
namespace extension {
@@ -65,3 +68,5 @@ torch_to_executorch_device(c10::Device device) {
6568

6669
} // namespace extension
6770
} // namespace executorch
71+
72+
C10_DIAGNOSTIC_POP()

extension/aten_util/targets.bzl

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

10-
# Split out of :aten_bridge so that ATen-mode code can convert devices.
11-
# :aten_bridge itself only compiles in portable mode.
10+
# Split out of :aten_bridge, which compiles in portable mode only, so that
11+
# ATen mode code can still convert devices.
1212
runtime.cxx_library(
1313
name = "aten_device",
1414
exported_headers = ["aten_device.h"],

runtime/core/portable_type/targets.bzl

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,9 @@ def define_common_targets():
4646
],
4747
)
4848

49-
# Device is the one portable type that ATen-mode code also needs, so it is
50-
# its own library: depending on :portable_type from an ATen-mode target
51-
# would drag in the portable Tensor alongside at::Tensor.
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.
5252
runtime.cxx_library(
5353
name = "device",
5454
exported_headers = [

0 commit comments

Comments
 (0)