Fix a few JITLink nits #46216
Fix a few JITLink nits #46216
Conversation
pchintalapudi
added
compiler:codegen
pchintalapudi
force-pushed
the
pc/jitlink-nits
branch
from
c7fcc25 to
02dd1df
Compare
| @@ -8603,7 +8603,7 @@ extern "C" void jl_init_llvm(void) | |||
| defined(JL_USE_OPROFILE_JITEVENTS) || \ | |||
| defined(JL_USE_PERF_JITEVENTS) | |||
| #ifdef JL_USE_JITLINK | |||
| #error "JIT profiling support (JL_USE_*_JITEVENTS) not yet available on platforms that use JITLink" | |||
| #pragma message("JIT profiling support (JL_USE_*_JITEVENTS) not yet available on platforms that use JITLink") | |||
There was a problem hiding this comment.
I don't have a strong opinion here, but my thinking behind making it an error was that the USE_*_JITEVENTS flags need to be explicitly specified by the user, so if they can't be honoured, they shouldn't be silently ignored.
There was a problem hiding this comment.
For whatever reason, at least on Linux-x86-64 the default make command sets one or more of these flags, so I turned it into a message so that I could test JITLink on that platform. Since at least for the time being we're not enabling JITLink on any platform except the ones that actually need it (aarch64), I thought it would be better as a message rather than an error.
There was a problem hiding this comment.
Yes on Linux we ship perf support by default.
There was a problem hiding this comment.
Ah, right – but couldn't we then rather just change those defaults for aarch64-linux once that is switched over as well, as that would be self-documenting for users in combination with doc/src/manual/profile.md? Maybe a compiler message is visible enough, though, I haven't tried.
| for (const jitlink::Section &Sec : G.sections()) { | ||
| #ifdef _OS_DARWIN_ |
|
I'll have to leave the locking (+docs) changes for somebody else to review, as I don't have a good overview of what the evolution of the codegen locking strategy is intended to look like (as long as we don't start generating code from multiple threads, the extra fine-grained locking is just clutter, hence I left it out originally). |
|
LGTM |
This PR steals the JITLink improvements from #45859, and also introduces a mutex to the debuginfo JITLink plugin to make it concurrency-safe. It also updates the locking documentation.