-
Notifications
You must be signed in to change notification settings - Fork 370
Fixes for building onnx-mlir on Windows with Visual Studio 2022 #3253
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
base: main
Are you sure you want to change the base?
Conversation
647edc7
to
db2c7aa
Compare
Can one of the admins verify this patch? |
@jenkins-droid test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for fixing the Windows build.
Is this a one-off attempt, or are you looking to maintain Windows support?
I'm still getting my bearings in the Onnx and MLIR space, so it's a one-off attempt at the minute. |
Signed-off-by: Mark Schofield <[email protected]>
Signed-off-by: Mark Schofield <[email protected]>
Signed-off-by: Mark Schofield <[email protected]>
Signed-off-by: Mark Schofield <[email protected]>
8640c5d
to
b127c21
Compare
Can one of the admins verify this patch? |
@jenkins-droid test this please |
Whilst trying out onnx-mlir I hit a few problems getting things compiling on Windows. This PR contains some of the changes I needed to make to get onnx-mlir working correctly - I'm still working through the (I think) last issue. I'm sending these changes in a single PR to show the scope of the changes - I'm happy to break individual commits out and submit them separately if that's preferred. The commits are:
Add 'ONNX_MLIR_ENABLE_OMP' to allow explicit opt-out of OpenMP support - I followed the instructions for building on Windows - which opt-out of openmp support for LLVM - yet my build has the 'omp' target declared. The comment in
src/Runtime/omp/CMakeLists.txt
suggests that that shouldn't be the case, but my build ends-up building the 'OMomp' project, which doesn't work on Windows. I've added aONNX_MLIR_ENABLE_OMP
option to explicitly opt-out. I updated the Windows documentation and.cmd
file to opt-out of OpenMP support.Define M_PI if it isn't already defined -
src/Conversion/ONNXToKrnl/Math/Window.cpp
assumes thatM_PI
is defined. It's not a C or C++ standard define. On Windows you can get the compiler to define it by setting the pre-processor define_USE_MATH_DEFINES
, but adding one non-standard definition to get another non-standard definition seems a bit awkward, so I just added a protected definition ofM_PI
.Don't rely on implicit std::filesystem::path --> std::string conversion -
std::filesystem::path
has implicit conversion tostd::filesystem::path::string_type
, wherestring_type
is the string type that is native to the character set of the file-system. On (most?) non-Windows platforms, that would bestd::string
, but on Windows that'sstd::wstring
. Rather than relying on implicit conversion, call.string()
.Suppress warning 4927 around the consumption of ONNXConstProp.inc - MSVC warns:
And since warnings are treated as errors, this breaks the build. Since the warning is in the generated code, the tightest scope I can suppress at is around the
#include
of the generated code, and that's what this commit does.