[CMake][MSVC] Disable permissive mode for MSVC builds #16343

Lunderberg · 2024-01-03T20:36:37Z

The C++ standard requires two-phase name resolution for templates. By default, MSVC uses a non-standard name resolution, in which all names are looked up when a template is instantiated. This has caused MSVC-specific compilation errors (example), which are quite difficult to debug.

This commit updates adds the /permissive- flag when building TVM with MSVC, disabling the non-standard name resolution.

The C++ standard requires two-phase name resolution for templates. By default, MSVC uses a non-standard name resolution, in which all names are looked up when a template is instantiated. This has caused MSVC-specific compilation errors, ([example](https://github.com/apache/tvm/actions/runs/7400684492/job/20134841480?pr=16183)), which are quite difficult to debug. This commit updates adds the `/permissive-` flag when building TVM with MSVC, disabling the non-standard name resolution.

junrushao · 2024-01-04T17:56:33Z

I don't fully understand this patch, but seems that it solves a debuggability issue with MSVC. Is there any downsides for this change?

Lunderberg · 2024-01-04T18:18:10Z

The only potential downside would be if we have any code that depends on the non-standard name resolution used by MSVC. Most of the differences would result in compile-time failures, but there are some very weird edge cases that can occur. (e.g. Suppose void func(int) is declared before the template, and void func(bool) is declared after the template but before the template instantiation. Inside the template, a call to func(true) should resolve to void func(int), because name resolution happens when the template is parsed. However, MSVC would silently and erroneously resolve it to void func(bool), because it does the name lookup at instantiation time.)

Even though two-phase name resolution has been the C++ standard as long as C++ has had a standard, MSVC still defaults to the non-standard name resolution for backwards compatibility with pre-C++98 code. Their worry is that switching to standard name resolution would silently switch behavior that was developed and compiled only on MSVC. Given that we primarily compile, test, and run on gcc/clang builds, I think this is very unlikely that we rely on MSVC's behavior, and any differences are most likely bugs in the MSVC build.

Lunderberg · 2024-01-04T18:19:13Z

TL;DR: Very unlikely to introduce a bug, and any reliance on this behavior would have caused bugs in the gcc builds.

junrushao · 2024-01-04T19:21:26Z

Got it. Yeah I do think it's very unlikely that people rely on MSVC-specific name resolution behavior. Thanks for clarifying this with me!

junrushao

LGTM!

- Revert "[CMake][MSVC] Disable permissive mode for MSVC builds (apache#16343)" - Skip MSC tests - Disable NNPack and TFLite - Tweak CMAKE_CUDA_ARCHITECTURES

Prior to this commit, various header files had `using namespace tvm::runtime`, which imports all names from `tvm::runtime` into the current namespace. These imports can cause compilation errors depending on the order of `#include` statements. For example, the `#include <tvm/relay/attrs/transform.h>` file uses the unqualified name `Bool` to refer to `::tvm::Bool`, a subclass of `PrimExpr`. If a different header file specifies `using namespace tvm::runtime` within the `tvm::relay` namespace, then the unqualified name `Bool` ambiguously refers to either `::tvm::Bool` or `::tvm::runtime::Bool`. In MSVC, this can cause even further compilation errors. By default, MSVC does not follow the C++ standard for name resolution in templates. The standard requires that any names in a template that do not depend on template parameters be resolved when the template is declared. However, MSVC instead resolves these names when the template is instantiated. As a result, the same `using namespace tvm::runtime` may cause a compilation error if it occurs after the template's declaration, but before the template's usage. (TVM provides the `/permissive-` flag to MSVC builds specifically to disable MSVC's non-standard name resolution, so this only impacts downstream forks that disable this flag. See apache#16343 for more details.) This commit removes `using namespace tvm::runtime`, replacing them with explicit `using tvm::runtime::SOME_SPECIFIC_SYMBOL` where necessary. This resolves both the include-order dependency for standards-compliant compilers, and the compilation errors for MSVC's default build.

- Revert "[CMake][MSVC] Disable permissive mode for MSVC builds (apache#16343)" - Skip MSC tests - Disable NNPack and TFLite - Tweak CMAKE_CUDA_ARCHITECTURES

Re-apply the change from upstream apache#16343. See apache#17246 for an example of the type of bug that can result from MSVC's default non-standard name resolution.

Prior to this commit, various header files had `using namespace tvm::runtime`, which imports all names from `tvm::runtime` into the current namespace. These imports can cause compilation errors depending on the order of `#include` statements. For example, the `#include <tvm/relay/attrs/transform.h>` file uses the unqualified name `Bool` to refer to `::tvm::Bool`, a subclass of `PrimExpr`. If a different header file specifies `using namespace tvm::runtime` within the `tvm::relay` namespace, then the unqualified name `Bool` ambiguously refers to either `::tvm::Bool` or `::tvm::runtime::Bool`. In MSVC, this can cause even further compilation errors. By default, MSVC does not follow the C++ standard for name resolution in templates. The standard requires that any names in a template that do not depend on template parameters be resolved when the template is declared. However, MSVC instead resolves these names when the template is instantiated. As a result, the same `using namespace tvm::runtime` may cause a compilation error if it occurs after the template's declaration, but before the template's usage. (TVM provides the `/permissive-` flag to MSVC builds specifically to disable MSVC's non-standard name resolution, so this only impacts downstream forks that disable this flag. See apache#16343 for more details.) This commit removes `using namespace tvm::runtime`, replacing them with explicit `using tvm::runtime::SOME_SPECIFIC_SYMBOL` where necessary. This resolves both the include-order dependency for standards-compliant compilers, and the compilation errors for MSVC's default build.

Prior to this commit, various header files had `using namespace tvm::runtime`, which imports all names from `tvm::runtime` into the current namespace. These imports can cause compilation errors depending on the order of `#include` statements. For example, the `#include <tvm/relay/attrs/transform.h>` file uses the unqualified name `Bool` to refer to `::tvm::Bool`, a subclass of `PrimExpr`. If a different header file specifies `using namespace tvm::runtime` within the `tvm::relay` namespace, then the unqualified name `Bool` ambiguously refers to either `::tvm::Bool` or `::tvm::runtime::Bool`. In MSVC, this can cause even further compilation errors. By default, MSVC does not follow the C++ standard for name resolution in templates. The standard requires that any names in a template that do not depend on template parameters be resolved when the template is declared. However, MSVC instead resolves these names when the template is instantiated. As a result, the same `using namespace tvm::runtime` may cause a compilation error if it occurs after the template's declaration, but before the template's usage. (TVM provides the `/permissive-` flag to MSVC builds specifically to disable MSVC's non-standard name resolution, so this only impacts downstream forks that disable this flag. See #16343 for more details.) This commit removes `using namespace tvm::runtime`, replacing them with explicit `using tvm::runtime::SOME_SPECIFIC_SYMBOL` where necessary. This resolves both the include-order dependency for standards-compliant compilers, and the compilation errors for MSVC's default build.

- Revert "[CMake][MSVC] Disable permissive mode for MSVC builds (apache#16343)" - Skip MSC tests - Disable NNPack and TFLite - Tweak CMAKE_CUDA_ARCHITECTURES

junrushao approved these changes Jan 4, 2024

View reviewed changes

junrushao merged commit e3d031b into apache:main Jan 4, 2024

Lunderberg deleted the msvc_disable_permissive_mode branch January 4, 2024 19:30

rickzx pushed a commit to rickzx/tvm that referenced this pull request Feb 19, 2024

[MLC] CI Tweaks

f8b2ff1

- Revert "[CMake][MSVC] Disable permissive mode for MSVC builds (apache#16343)" - Skip MSC tests - Disable NNPack and TFLite - Tweak CMAKE_CUDA_ARCHITECTURES

echuraev pushed a commit to echuraev/tvm that referenced this pull request Apr 8, 2024

[MLC] CI Tweaks

59c3556

- Revert "[CMake][MSVC] Disable permissive mode for MSVC builds (apache#16343)" - Skip MSC tests - Disable NNPack and TFLite - Tweak CMAKE_CUDA_ARCHITECTURES

Ubospica pushed a commit to Ubospica/tvm-develop that referenced this pull request Apr 18, 2024

[MLC] CI Tweaks

aacd9c4

- Revert "[CMake][MSVC] Disable permissive mode for MSVC builds (apache#16343)" - Skip MSC tests - Disable NNPack and TFLite - Tweak CMAKE_CUDA_ARCHITECTURES

ysh329 mentioned this pull request Apr 21, 2024

[Release] v0.16.0 Release Candidate Notes #16911

Closed

Lunderberg mentioned this pull request Aug 6, 2024

[Cleanup] Remove using namespace tvm::runtime from headers #17246

Merged

Lunderberg pushed a commit to Lunderberg/tvm that referenced this pull request Aug 6, 2024

[MLC] CI Tweaks

c25ab6f

- Revert "[CMake][MSVC] Disable permissive mode for MSVC builds (apache#16343)" - Skip MSC tests - Disable NNPack and TFLite - Tweak CMAKE_CUDA_ARCHITECTURES

MasterJH5574 added a commit to MasterJH5574/tvm that referenced this pull request Oct 26, 2024

[MLC] CI Tweaks

c29d59d

- Revert "[CMake][MSVC] Disable permissive mode for MSVC builds (apache#16343)" - Skip MSC tests - Disable NNPack and TFLite - Tweak CMAKE_CUDA_ARCHITECTURES

srkreddy1238 pushed a commit to CodeLinaro/tvm that referenced this pull request May 6, 2025

[MLC] CI Tweaks

74b2177

- Revert "[CMake][MSVC] Disable permissive mode for MSVC builds (apache#16343)" - Skip MSC tests - Disable NNPack and TFLite - Tweak CMAKE_CUDA_ARCHITECTURES

kimm240 pushed a commit to kimm240/tvm that referenced this pull request Nov 4, 2025

[MLC] CI Tweaks

69a162f

- Revert "[CMake][MSVC] Disable permissive mode for MSVC builds (apache#16343)" - Skip MSC tests - Disable NNPack and TFLite - Tweak CMAKE_CUDA_ARCHITECTURES

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[CMake][MSVC] Disable permissive mode for MSVC builds #16343

[CMake][MSVC] Disable permissive mode for MSVC builds #16343

Uh oh!

Lunderberg commented Jan 3, 2024

Uh oh!

junrushao commented Jan 4, 2024

Uh oh!

Lunderberg commented Jan 4, 2024

Uh oh!

Lunderberg commented Jan 4, 2024

Uh oh!

junrushao commented Jan 4, 2024 •

edited

Loading

Uh oh!

junrushao left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[CMake][MSVC] Disable permissive mode for MSVC builds #16343

[CMake][MSVC] Disable permissive mode for MSVC builds #16343

Uh oh!

Conversation

Lunderberg commented Jan 3, 2024

Uh oh!

junrushao commented Jan 4, 2024

Uh oh!

Lunderberg commented Jan 4, 2024

Uh oh!

Lunderberg commented Jan 4, 2024

Uh oh!

junrushao commented Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junrushao left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

junrushao commented Jan 4, 2024 •

edited

Loading