This is an archive of the discontinued LLVM Phabricator instance.

[ExecutionEngine] Track objects using an abstract ObjectKey in JITEventListeners.
ClosedPublic

Authored by lhames on Oct 26 2018, 12:01 PM.

Details

Summary

The aim of this patch is to enable JITEventListeners to work with ORC without
forcing us to keep object files alive longer than necessary.

Currently JITEventListener::NotifyObjectEmitted and NotifyObjectFreed take a
reference to the object file being emitted, but in NotifyObjectFreed this is
only ever used as a key to deallocate resources that were allocated in
NotifyObjectEmitted. Switching to an abstract key allows the JIT to deallocate
object file buffers after NotifyObjectEmitted has been called.

No guarantee is made about the key value, except that it will be distinct for
each object that has been allocated. Keys may be re-used after they are freed.

Diff Detail

Event Timeline

lhames created this revision.Oct 26 2018, 12:01 PM
lhames updated this revision to Diff 171390.Oct 26 2018, 9:35 PM
  • [ExecutionEngine] Rename/format JITEventListener methods.
anarazel requested changes to this revision.Oct 30 2018, 1:11 PM

This looks like a sensible improvement. There's probably not many implementors of the interface, so the renaming shouldn't cause much pain.

I noticed however that you didn't update ExecutionEngine/PerfJITEvents/PerfJITEventListener.cpp?

lib/ExecutionEngine/MCJIT/MCJIT.cpp
654

Perhaps this should rather live inside ObjectFile? No strong opinion, just pondering. Seems a bit prettier than having the ugly multiple cast in multiple places.

This revision now requires changes to proceed.Oct 30 2018, 1:11 PM

This looks like a sensible improvement. There's probably not many implementors of the interface, so the renaming shouldn't cause much pain.

I noticed however that you didn't update ExecutionEngine/PerfJITEvents/PerfJITEventListener.cpp?

Oops -- I totally missed that. I will fix it and post an updated patch tomorrow.

lib/ExecutionEngine/MCJIT/MCJIT.cpp
654

Unfortunately this can not be moved to ObjectFile: For MCJIT it makes sense to continue using the original ObjectFile address as the key, but for ORC the aim is to decouple the key from the ObjectFile so that the latter can be deallocated.

andrew.w.kaylor added inline comments.Nov 2 2018, 3:46 PM
include/llvm/ExecutionEngine/JITEventListener.h
53

The comment is still using the old name here.

lhames updated this revision to Diff 173947.Nov 13 2018, 3:24 PM

Updates the PerfJITEventListener API and fixes a comment that referred to the old API.

Gentle ping.

This is not high priority yet, but if there are no further issues with it I would like to land it as I intend to build on it in the future.

anarazel accepted this revision.Nov 28 2018, 3:04 PM

Gentle ping.

This is not high priority yet, but if there are no further issues with it I would like to land it as I intend to build on it in the future.

Sorry, missed the updated version. I don't know if my ack is sufficient to move this forward, but the update certainly fixes the complaint I had. And the rest looks reasonable.

This revision is now accepted and ready to land.Nov 28 2018, 3:04 PM
andrew.w.kaylor accepted this revision.Nov 28 2018, 3:19 PM

Looks good to me too.

lhames closed this revision.Dec 6 2018, 6:14 PM

Committed in r348223.