Repository navigation
Commit d5b5a92
committed
Pin the CUDA fallback kernel the backend just started advertising
The CUDA test that asserts the exact set of fallback kernels fails:
test_cuda_fallbacks_unchanged_by_rocm_gate
An earlier commit taught the backend to accept the shim spelling Inductor now
derives for the int4 pack matmul, but this test compares the advertised set
against a frozen copy of it, so adding an entry on one side alone breaks the
comparison. Add the same entry here.
The Metal backend took the same kind of change and is unaffected, because nothing
pins its set exactly.
Test Plan:
Extracted both sets and compared them directly. Before this the advertised set has
twelve entries and the frozen copy eleven, with the new derived spelling the only
difference and nothing frozen that the backend no longer advertises, so the
comparison fails on exactly that one name. After, both hold twelve and the
comparison passes.
Treat a test that names c10 as an ATen test too
The Buck test targets decide their C++ standard separately from the libraries, and
their check for "is this an ATen test" looks for a dependency named `libtorch` but
not for the other two names the build maps onto the same library. A test that names
`c10` or `torch-core-cpp` was therefore pinned to C++17 and failed on ATen's
headers, which need C++20 as of PyTorch 2.14:
ATen/core/TensorBase.h: error: expected ';' at end of declaration
Reuse the same check the library path already uses, so both paths agree on what
counts as ATen. It covers all three dependency names, and the exported list as well
as the plain one.
The rest of the test check is left alone: a name containing `aten`, and the ATen
flavours of the test framework, still mark a test as ATen on their own.
Test Plan:
Ran the wrapper function directly, with `type()` behaving as Starlark's does, over
the cases that matter. Before this a test naming `c10` or `torch-core-cpp` came back
with C++17; after, both come back without a pin and take C++20 from the toolchain.
A plain test with no ATen dependency still comes back with C++17, which is the case
the embedded build depends on, and a test named for ATen or naming `libtorch` is
unchanged.
Moved the shared check above its first use, since it now has one earlier in the
file, and dropped a local that nothing reads any more.
Put the C++20 flag where the compiler actually reads it
The Buck build still fails compiling anything generated that includes ATen:
ATen/core/TensorBase.h:1031:5: error: unknown type name 'requires'
`requires` is a C++20 keyword, so the compile was still running at C++17 even
though the target is ATen-mode and the wrapper had already decided it should be
C++20.
The wrapper was setting the standard only in the per-language flags, which reach
the preprocessor pass. That was enough while the value matched the build config,
which pins C++17 for everything, and it is not enough to override it. Set the flag
on the target's own compile flags as well, for ATen-mode targets only.
Nothing changes for the rest of the build: a target that is not ATen-mode gets no
flag added here and keeps the C++17 the build config gives it.
Test Plan:
Ran the wrapper over the exact arguments the generated kernel library is defined
with, the ones from the failure. Before this it came away with only its own
warning flag and the standard confined to the per-language entry, which is why the
compile stayed at C++17. After, the flag is on the target's own list where the
compile reads it.
Checked the case that must not move: a library with no ATen dependency still gets
C++17 and no flag on its own list.
Confirmed why the old spelling looked sufficient. It only ever carried C++17,
matching the build config, so nothing had shown it could raise a standard rather
than restate one.
I cannot run this Buck build on this machine, so CI has to confirm the generated
libraries now compile.
Raise the ATen-mode C++ standard where both builds see it
The C++20 flag for ATen-mode targets was set in the interface layer that exists
only for the open source build, whose own docstring says as much. So the internal
build never got it, and the shared wrapper was passing that layer an argument its
internal counterpart does not accept, which would fail there rather than being
merely ineffective.
Move the decision into the shared wrapper, which both builds go through, and leave
the interface layer exactly as it was. The flag goes on the target's own compile
flags, because the build config applies C++17 to everything and only a flag on the
target overrides it.
Test Plan:
Read the two files' own descriptions of themselves to place this: the interface
layer is for changes specific to one build, the wrapper is for logic shared with
the internal one.
Confirmed the internal counterpart takes only the arguments it always took, so the
previous version would have broken it rather than skipped it.
Ran the new helper over the target from the failure and over targets that must not
move. The generated kernel library and the ATen bridge come away with C++20; a
plain library and the portable kernels come away with no flag added, so they keep
the C++17 the build config gives them, which is what the embedded builds rely on.
I cannot run either Buck build here, so CI has to confirm the generated libraries
compile.
Decide ATen mode before the wrapper consumes the dependency list
The Buck build still failed compiling generated sources that include ATen:
ATen/core/TensorBase.h:1031:5: error: unknown type name 'requires'
The check for "is this an ATen target" reads the external dependency list, but by
the time it ran, an earlier step in the same wrapper had already removed that list
after turning its entries into real dependencies. So every target looked like a
non-ATen one, no target was ever raised to C++20, and the flag added for it was
never added at all.
Read the list first, then let the rest of the wrapper consume it.
Test Plan:
Traced the order in the wrapper: the step that removes the list runs before the
step that read it. Reproduced that by running both against the target from the
failure: it reports ATen mode with the list present and not once it is gone, which
is exactly the sequence the wrapper produced.
Reran the whole path in the new order over the target from the failure, an ATen
target that exports its dependency instead of naming it directly, and a plain
library. The first two now come away at C++20 and the plain one is untouched, so
the embedded builds keep the C++17 they rely on.
Also read the build system's own source to confirm the flag can win where it is
placed: a target's flags are appended after the toolchain's, and its comment says
they are last on purpose. So the earlier concern that the flag was being placed
somewhere that could not take effect was unfounded; it simply was not being added.
Shortened an overlong comment that failed the formatter.
I cannot run this Buck build here, so CI has to confirm the generated libraries
compile.1 parent 8d79988 commit d5b5a92
4 files changed
Lines changed: 34 additions & 29 deletions
File tree
- backends
- aoti
- cuda/tests
- shim_et/xplat/executorch/build
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
62 | 62 | | |
63 | 63 | | |
64 | 64 | | |
65 | | - | |
66 | | - | |
| 65 | + | |
67 | 66 | | |
68 | 67 | | |
69 | 68 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
37 | 37 | | |
38 | 38 | | |
39 | 39 | | |
| 40 | + | |
40 | 41 | | |
41 | 42 | | |
42 | 43 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
160 | 160 | | |
161 | 161 | | |
162 | 162 | | |
163 | | - | |
164 | | - | |
165 | | - | |
166 | | - | |
167 | | - | |
168 | | - | |
169 | | - | |
170 | | - | |
171 | | - | |
| 163 | + | |
| 164 | + | |
172 | 165 | | |
173 | | - | |
| 166 | + | |
174 | 167 | | |
175 | | - | |
| 168 | + | |
176 | 169 | | |
177 | | - | |
| 170 | + | |
178 | 171 | | |
179 | 172 | | |
180 | 173 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
145 | 145 | | |
146 | 146 | | |
147 | 147 | | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
148 | 160 | | |
149 | 161 | | |
150 | 162 | | |
| |||
154 | 166 | | |
155 | 167 | | |
156 | 168 | | |
157 | | - | |
158 | 169 | | |
159 | 170 | | |
160 | 171 | | |
161 | 172 | | |
162 | 173 | | |
163 | | - | |
164 | 174 | | |
165 | 175 | | |
166 | | - | |
| 176 | + | |
167 | 177 | | |
168 | 178 | | |
169 | 179 | | |
| |||
267 | 277 | | |
268 | 278 | | |
269 | 279 | | |
270 | | - | |
271 | | - | |
| 280 | + | |
| 281 | + | |
272 | 282 | | |
273 | | - | |
274 | | - | |
| 283 | + | |
275 | 284 | | |
276 | | - | |
277 | | - | |
278 | | - | |
279 | | - | |
280 | | - | |
281 | | - | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
282 | 288 | | |
283 | 289 | | |
| 290 | + | |
| 291 | + | |
284 | 292 | | |
285 | 293 | | |
| 294 | + | |
286 | 295 | | |
287 | 296 | | |
288 | 297 | | |
289 | 298 | | |
290 | | - | |
| 299 | + | |
291 | 300 | | |
292 | 301 | | |
293 | 302 | | |
| |||
307 | 316 | | |
308 | 317 | | |
309 | 318 | | |
| 319 | + | |
| 320 | + | |
310 | 321 | | |
311 | 322 | | |
| 323 | + | |
312 | 324 | | |
313 | | - | |
| 325 | + | |
314 | 326 | | |
315 | 327 | | |
316 | 328 | | |
| |||
0 commit comments