Native Windows Compile Fixes For vpr
#1582
Conversation
As posix_memalign does not exist on Windows, we can use the above functions as a workaround until C++20 support matures.
Use uintptr_t in parser file- the warning is outdated. For all other conversions, intptr_t is chosen as the equivalent of long used as a pointer on LP64, and the equivalent of long long on LLP64.
sys/mman.h is not required to compiler libvtrcapnproto. To keep changes minimally invasive, headers such as unistd.h are retained. These headers may only exist as part of the MinGW-w64 compiler on Windows and not MSVC.
| @@ -360,7 +361,7 @@ t_pin_def *add_pin(char *name, int left, int right, t_pin_def_type type, t_parse | |||
|
|
|||
| //Append the element, the position is one less thatn the size of the array | |||
| //which is returned by append_array_element | |||
| size_t position = append_array_element( (long) pin, pin_list) - 1; | |||
| size_t position = append_array_element( (intptr_t) pin, pin_list) - 1; | |||
|
|
|||
There was a problem hiding this comment.
I don't think you should change long to intptr_t. We're using id-based structures in the code I looked at below, where we are inserting things into vectors, setting integer ids and such. We don't need a pointer for that; just an integer of sufficient size. So long is appropriate for most cases, and for some smaller ids we use shorts.
There was a problem hiding this comment.
long => int64_t then? The problem with long on Windows is that it's not the same size as a pointer on 64-bit windows (long is 32 bit). So all the conversions from pointer to an integer will truncate (and cause compiler errors). What about a typedef such as id_t to convey meaning?
As for the compiler errors, I'll take a look. At least for the one you showed, looks like I forgot a header...
Also, Idk if signedness actually matters, but I've been using signed numbers when possible to match where the code uses signed numbers.
There was a problem hiding this comment.
@vaughnbetz Friendly ping... what should I do instead of changing long to intptr_t? Keeping long here will not work because long is not 64-bits long, even on 64-bit Windows. So I need to change the type to something that will be the same size as a pointer (since we cast between pointer and int) on all platforms vpr runs on. Unless you prefer a platform-specific typedef, intptr_t seems to be the most appropriate type.
However, I am unable to test immediately on Linux.
I am now able to test on Linux :), so I'll make sure I can compile locally before pushing new changes. Should I rebase to remove the intptr_t changes from history (if applicable), or leave as-is and keep committing on-top? Gonna guess rebase is appropriate since I broke the build, but unsure.
|
There are a lot of build errors (see the CI above; here is an example): 77%] Building CXX object libs/libvqm/CMakeFiles/libvqm.dir/vqm_dll.cpp.o I also think this PR makes many unnecessary changes of long to intptr_t that aren't needed and obscure meaning. To do a quick test on windows you could manually run vpr using the commands at https://docs.verilogtorouting.org/en/latest/quickstart/ For Linux, you would need to pass the full CI, which is run automatically on each PR (and has been run above but had failures). |
|
OK, I stand corrected on intptr_t. I talked to Kevin Murray today and he said libvqm is weird; it is indeed using long for pointers. Sorry, I didn't catch that your long->intptr_t changes were all in libvqm. We don't use long for pointers in the main vpr code base, so I thought you were changing ids to intptr_t. So the libvqm changes look good; go for them. The other changes outside libvqm look limited and reasonble. So if you can get CI green, and confirm manually that vpr builds & functions on windows then we can merge this. Longer term, we should get Windows builds going on Travis CI. If you also want to take a crack at that feel free, but antmicro will take putting Windows builds into CI if you get them working, so you don't have to do that if you don't know travis setup. |
This is required because it is not guaranteed that stdint.h will be in scope by the C++ compiler by the time the parser data types are included. The parser union, in particular, needs uintptr_t.
|
Results of running the quickstart on Windows match the expected output, so I think VPR is working fine. Linux compile works fine now too. Now waiting on CI... |
|
Looks good except you need to run 'make format' to format the code to specification, and then check in the updated files (it'll only be the ones you changed). That's the only CI failure I see. |
|
@vaughnbetz I only have |
|
EDIT: @mithro You are correct, there is a diff. I somehow managed to miss that even after your comment :P! |
|
CI looks stuck; forcing a kokoro rerun. |
|
CI continues to look stuck :(... |
|
I kicked the kokoro build off again. |
|
Kokoro still has no output ... strange. Seems to be working on other PRs. |
|
OK, done. Here's hoping! |
Description
This PR adds some fixes so that
mingw-w64gcccan successfully compilervpr. Onlyvpris known to compile as of this PR.Specifically:
_aligned_mallocand_aligned_freeon Windows instead oflongtouintptr_torintptr_tbased on context inlibvqm.sys/mman.hheader inlibvtrcapnproto.Related Issue
This PR should close #1317, but includes additional fixes so that compilation succeeds using
gcctargeting native windows exes.Motivation and Context
Per @vaughnbetz in #1317, getting a Windows compile without Cygwin is desired, but hasn't worked in a few months.
How Has This Been Tested?
See below in Checklist for how I tested changes.
longtouintptr_torintptr_tshould be a no-op on LP64 systems like Linux (and thus have no effect on the compiled code). However, I am unable to test immediately on Linux.Environment:
CMake command line:
System:
Compiler:
Types of changes
Checklist:
First three are not applicable. As for the tests...
I have only compiled
vpr, as that is at present the only part ofvtrthat I plan to work on. When I try runningpython run_reg_test.py vtr_reg_basic, the test hangs without any output, most likely because I only installedvprand not the rest (fwiw,abcis known to compile fine on Windows).However, when I run
test_vpr, andtest_fasm, I get the following output:test_vpr:test_fasm:Looks like most things are working, though it appears that the test binaries don't like me running them directly from the build directory. How should I go about testing with these binaries properly?