Implement support for localloc. #417
Conversation
|
Note this requires these LLVM changes. So this won't go in until we've closed on those and have them available. |
| @@ -277,6 +277,10 @@ void GenIR::readerPrePass(uint8_t *Buffer, uint32_t NumBytes) { | |||
| ASSERTNR(UNREACHED); | |||
| } | |||
|
|
|||
| // We now require the LLVM default triple to end with | |||
| // -coreclr, eg x86_64-pc-win32-coreclr | |||
| ASSERT(PT.getEnvironment() == Triple::CoreCLR); | |||
There was a problem hiding this comment.
Where is the corresponding change that sets the target environment to coreclr?
|
@AndyAyersMS, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
|
@pgavlin Unless I'm confused there's no place in source control where we specify the target triple, other than docs. |
|
The corresponding change may be in LLVM, but surely the CoreCLR environment isn't the default when building LLVM? |
|
I don't think the LLVM build is particularly sensitive to the value of the default triple when it's built. So modifying LLVM's build seems like the wrong way to go about this. LLILC refers to LLVM_DEFAULT_TARGET_TRIPLE when setting up the module, this seems like the thing we should change. Instead of using the default we should have some other value we use here that is set up by LLILC's build scripts. |
|
Hence my question--I thought that you had made (or intended to make) such a change, and was confused by its absence :) |
|
Ok, good. |
|
Seems to work but could not figure out how to pass a string as a define value through CMake. So I'm currently stringizing via preprocessor magic.... Seem ok to you? I suspect there are macros like these in headers we include too so perhaps I can use something that is already there. |
|
@pgavlin PTAL at the updated changes. LLVM part is in already. |
| endif() | ||
|
|
||
| message(STATUS "LLILC_TARGET_TRIPLE is ${LLILC_TARGET_TRIPLE}") | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DLLILC_TARGET_TRIPLE=${LLILC_TARGET_TRIPLE}") |
There was a problem hiding this comment.
I think you can escape the quotes in order to get the value passed as a string: "-DLLILC_TARGET_TRIPLE=\"${LLILC_TARGET_TRIPLE}\""
There was a problem hiding this comment.
Tried that already and the quotes didn't make it through. I can experiment some more.
There was a problem hiding this comment.
It looks like you might need to escape the escapes. Not sure if there is a good cross-platform way to do that. For the Linux build, I expect that "-DFOO=\\\"${FOO}\\\"" would work, but CMD might need "-DFOO=^\"${FOO}^\"" or similar.
There was a problem hiding this comment.
If worst comes to worst and there's no good way to do this, your current workaround is fine.
There was a problem hiding this comment.
CMake's add_definitions seems a bit better about preserving quotes. So will go with that.
|
IR looks good, but I'd definitely like to avoid the extra macros if possible. |
Getting the LLVM IR right here is not too hard. We're zeroing via the JIT helper since using LLVM's memset might lead to calls into the CRT. Most of the challenge here is getting this IR properly lowered to machine code. I have some preliminary changes checked into LLVM for this already, and this change depends upon them. To enable those changes we now use the CoreCLR environment when targeting windows. Thus the LLVM triple we use on windows is now actually a quad. The LLVM work is incomplete so this only works at runtime if the localloc is small enough to stay within the guard page, but it handles all our simple test cases.
|
retest this please |
1 similar comment
|
retest this please |
Implement support for localloc.
|
CI failures look spurious, so am going to merge this. |
Getting the LLVM IR right here is not too hard. Note we're zeroing things via the helper since
using LLVM's memset might lead to calls into the CRT.
Most of the challenge here is in getting this IR properly lowered to machine code. I have some
corequisite changes to LLVM for this.