Disable mkl for windows builds #666
Conversation
| @@ -219,6 +219,9 @@ jobs: | |||
| $Env:BAZEL_VC = "C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC" | |||
| set PreferredToolArchitecture=x64 | |||
|
|
|||
| # The default is "/arch:AVX", which we don't want. | |||
| $Env:CC_OPT_FLAGS = "/O2" | |||
There was a problem hiding this comment.
Before adding this change, I did a test release that didn't pass: https://github.com/larq/compute-engine/actions/runs/943850235
But I don't know if that's because this change actually made a difference that makes the builds pass, or if I'm just observing high build-time noise (which anecdotally seems to be the case).
There was a problem hiding this comment.
I guess this could affect performance of the LCE interpreter on Windows, but:
a) We don't have optimised binary kernels for x86/AVX anyway.
b) AFAIK even without this flag AVX intrinsics will insert AVX instructions, so TFL kernels with optimised x86/AVX kernels should still be okay.
There was a problem hiding this comment.
Yeah I think this is a very reasonable thing to do.
| - "@org_tensorflow//tensorflow:linux_x86_64": if_true, | ||
| - "@org_tensorflow//tensorflow:windows": if_true, |
There was a problem hiding this comment.
This patch looks like its also disabling mkl for linux, not only for windows. Is that correct? (Anything that speeds up these builds sounds good to me)
There was a problem hiding this comment.
Yes, it disables it for both unless you request it through the relevant Bazel flags.
There was a problem hiding this comment.
Or at least, that's the intent. I haven't actually haven't tested on Linux whether this succeeds — if you run
bazel cquery "somepath(:build_pip_pkg, @mkl_dnn_v1//:mkl_dnn)" --define=build_with_mkl=false --define=enable_mkl=false --define=tensorflow_mkldnn_contraction_kernel=0
and get something that says "Empty query results" then MKL is successfully disabled.
| @@ -219,6 +219,9 @@ jobs: | |||
| $Env:BAZEL_VC = "C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC" | |||
| set PreferredToolArchitecture=x64 | |||
|
|
|||
| # The default is "/arch:AVX", which we don't want. | |||
| $Env:CC_OPT_FLAGS = "/O2" | |||
There was a problem hiding this comment.
Yeah I think this is a very reasonable thing to do.
This PR adds a Bazel patch to the TF repo that disables MKL for Windows and Linux (unless the
build_with_mkl/enable_mklBazel flags are provided).It also removes the
/arch:AVXbuild flag, which hopefully makes codegen a bit faster.The goal is to get our Windows release builds to successfully complete within the 6 hour time limit of GitHubActions.
A test release from this branch passes (with Windows build durations of 5h40, 5h28, 5h12, 4h50): https://github.com/larq/compute-engine/actions/runs/943927532.
The variance on these Windows build times does seem to be pretty high, and those times are still quite close to the 6 hour limit, so I'm not convinced that even after doing this we could rely on these builds passing.