-
Notifications
You must be signed in to change notification settings - Fork 406
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
Switch to use compiler param file for msvc #2843
Conversation
@ddunl can you ensure someone from your team is aware of this change how ever much you find appropriate? @jakeharmon8 I think you're the most knowledgeable on this after recent work, so can you share your thoughts on this change, even if you don't do the detailed review? |
Note, the <wrapper.bat> is added to workaround # msvc_cl_path
# /
python.exe -B <wrapper.py> [[option1]...]
# \--- write to file.params ---/
# then invoked with
python.exe @file.params # This is ill-formed!!!!!!
# file.params
-B
<wrapper.py>
option1
... with # <wrapper.bat>
python.exe -B <wrapper.py> %s
# msvc_cl_path
# /
<wrapper.bat> [[option1]...]
# \--- write to file.params
# then invoked with
<wrapper.bat> @file.params
# file.params
option1
... |
@yashk2810 does this look good to you/any possible negative effect to JAX? |
@hawkinsp should probably look at this. |
I have no particular knowledge here. This looks good to me but @jakeharmon8 is probably the right person to review. Perhaps @tlongeri also? |
How about just trigger one run and see if it goes well? There is no external dependency at all and the logic is trivial as described in the comment #2843 (comment) |
I think Jake already looked at it and thought he wasn't the right person. In this case I'll try to merge! This one will require manual patching in due to issue with our copybara config with respect to tensorflow, and I'm out sick so likely won't get to it today. Thanks for your PR!! |
Today's my first day back, so I still may not get to patching this by the end of this week. If you check on this PR next week and there's no further activity, please tag me again! Sorry for the delay. |
Imported from GitHub PR openxla/xla#2843 Merging this change closes #2843 PiperOrigin-RevId: 536435983
Sorry for the delay, working on landing this now. I'll update with any issues should they arise |
Imported from GitHub PR openxla/xla#2843 Merging this change closes #2843 PiperOrigin-RevId: 536435983
Imported from GitHub PR openxla/xla#2843 Merging this change closes #2843 PiperOrigin-RevId: 536555386
Imported from GitHub PR openxla/xla#2843 Merging this change closes #2843 PiperOrigin-RevId: 536555386
These changes make the nvcc wrapper for msvc to fully support command file for msvc.
Previously, multiple
command is longer than CreateProcessW's limit (32767 characters)
errors pop up here and there due to the excessive_virtual_includes
introduced by mlir.This makes
<params_file> -->filter--> <msvc_param_file>
), that is in<wrapper.py>
, we callPopen(<msvc>, <msvc_param_file>)