assigned to @lhames

After rereading the code, I think that MCJMM could take over JMM role, since JITMemoryManager inherits from RTDyldMemoryManager and MCJMM could store both memory managers. That would save a whole lot of code in ExecutionEngine.cpp and ExecutionEngine.h which seem to serve no purpose now.

Very good point. Thanks for looking in to this Yaron!

I think setJITMemoryManager and the JMM member should be removed. Do you already have a patch for this? If not, I'm happy to get to work on it.

• Lang.

Hi Lang, I did not make a patch for it, wasn't initially 100% sure there isn't some use to the two different managers somewhere.

While at it, I also noticed JITMemoryManager (the class) is not not used anymore in any "core" llvm but only in lli.cpp and in llvm-jitlistener.cpp with the comment:

// FIXME: This is using the default legacy JITMemoryManager because it
// supports poison memory. At some point, we'll need to update this to
// use an MCJIT-specific memory manager. It might be nice to have the
// poison memory option there too.

The poison option is just memset

  if (PoisonMemory) {
    memset(MemRange+1, 0xCD, MemRange->BlockSize-sizeof(*MemRange));
  }

I'm not sure if lli.cpp really needs JITMemoryManager or could use another memory manager.

JITMemoryManager may be removed after fixing the two uses above.

Hi Yaron,

I'm going through all the references at the moment. I don't think lli needs JITMemoryManager. I'm not sure about llvm-jitlistener.cpp - that's up next.

Hopefully I'll have a patch for this shortly.

• Lang.

llvm-jiteventlistener.cpp doesn't currently compile. That's concerning.

Andy - Do you whether anyone at Intel is maintaining this code?

Cheers,
Lang.

I know that Arch Robinson (arch.robinson@intel.com) has been actively fixing regressions in the IntetlJITEventListener code as they arise because he is using it with the Julia language. One of his changes from a few weeks ago included a change to llvm-jiteventlistener.cpp. He's not an llvm committer, so Elena Demikhovsky committed the change on his befalf.

I'm not sure how long this will continue to be a priority for Arch. He's more attached to Julia than he is to llvm, and even that isn't his only responsibility. In general, if any problems arise that are specific to the IntelJITEventListener code, you should probably continue referring them to me and I'll figure out who I can send them to. I know that the VTune Analyzer team wants to keep this working, but unless it breaks they won't have anyone assigned to it and other than llvm interface changes that code has been fairly stable.

There doesn't seem to be sufficient coverage in the automated tests to catch even build failures for the JIT event listener stuff. The llvm-jiteventlistener tool was meant to help with that, but both the oprofile and the Intel-specific JIT event listeners only get enable with special build flags.

Could probably use a jit listener so you can get the size of a generated set of text and make sure it's > 0 for a quick and dirty "is it working" sort of thing.

Patch to remove the JITMemoryManager class.

Hi Andy,

The attached patch removes support for the JITMemoryManager from LLVM.

In the process it removes the use of poison values for allocated/freed memory in the llvm-jitlistener utility (since the RTDyldMemoryManagers don't support poison yet). As far as I can tell that will not actually impact the running of this utility, since nobody seems to be checking for the poison value. Does that sound right?

If there's no problem with the changes to llvm-jitlistener, I'll go ahead and commit this.

Cheers,
Lang.

Sounds good to me. The patch also looks right. My only suggestion would be to change the error checking so that instead of reporting an error if remote jit is requested without -use-mcjit you just have remote jit implicitly set use-mcjit.

Thanks for the review Andy.

I've removed the JITMemoryManager class and its associated APIs in r218316 (with some fallout that was cleaned up in r218317 and r218318).

Since using MCJIT is now the default I've left the error message in to catch the case where the user explicitly requests -force-interpreter -remote-mcjit.

Cheers,
Lang.