Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows related changes in CMakeLists.txt #5186

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

anmyachev
Copy link
Contributor

Upstreaming some of our Windows related changes assuming that there is interest in this #5094 (comment) and hoping that it will not make it much more difficult to support this CMake file.

CMakeLists.txt Outdated
if(NOT MSVC)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D__STDC_FORMAT_MACROS -fPIC -std=gnu++17")
else()
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D__STDC_FORMAT_MACROS /wd4244 /wd4624 /wd4715 /wd4530")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: can you put all the error-related flags on line 139

CMakeLists.txt Outdated
set(CMAKE_CXX_FLAGS_TRITONBUILDWITHO1 "-O1")
else()
set(CMAKE_C_FLAGS_TRITONRELBUILDWITHASSERTS "/Zi /Ob0 /Od /RTC1 /bigobj /Zc:preprocessor")
set(CMAKE_CXX_FLAGS_TRITONRELBUILDWITHASSERTS "/Zi /Ob0 /Od /RTC1 /bigobj /Zc:preprocessor")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/Ob0 and /Od are debug flags, not release?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, removed

Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev
Copy link
Contributor Author

@peterbell10 thanks for review! Ready for another round.

If I remember correctly we tried to compile the code and -std=gnu++17 flag was not needed. Could it be that this is just an artifact of the past and I can try to remove it? Or would you prefer to leave it anyway?

@peterbell10
Copy link
Contributor

If it's not needed then I'd be fine with removing it. I guess #4976 removed the only use of builtins?

@woct0rdho
Copy link

I just want to say that if we remove -std=gnu++17 and allow to use C++20, it would be much easier to build on Windows using MSVC (MSVC is usually needed to build with CUDA), because some namespace resolution behaviors in code like lib/Analysis/Utility.cpp become standard

@anmyachev
Copy link
Contributor Author

If it's not needed then I'd be fine with removing it. I guess #4976 removed the only use of builtins?

There is at least one more biltins here:

int32_t trailingZeros = otherBits != 0 ? __builtin_ctz(otherBits) : 31;
Hm, it shouldn't work with c++17 only. Looks like set(CMAKE_CXX_STANDARD 17) for GCC implicitly use -std=gnu++17.

@anmyachev
Copy link
Contributor Author

I just want to say that if we remove -std=gnu++17 and allow to use C++20

@woct0rdho I'm looking forward to this event, but it's unlikely to happen before PyTorch switches to the new standard (at least that's my current understanding)

@peterbell10 I was wrong about -std=gnu++17 but it doesn't relate to this PR. Could you review it again?

@@ -306,6 +321,11 @@ if(NOT TRITON_BUILD_PYTHON_MODULE)
endforeach()
endif()

if(WIN32)
option(CMAKE_USE_WIN32_THREADS_INIT "using WIN32 threads" ON)
option(gtest_disable_pthreads "Disable uses of pthreads in gtest." ON)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be hard-coded or can we use find_package(Threads REQUIRED)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants