This is an archive of the discontinued LLVM Phabricator instance.

[ORC] Add deprecation warnings to ORCv1 layers and utilities.
ClosedPublic

Authored by lhames on Jul 11 2019, 5:58 PM.

Details

Summary

ORCv1 is deprecated and my current aim is to remove it after LLVM 9.0. This
patch adds deprecation attributes to the ORCv1 layers and utilities to warn
clients of the change.

Diff Detail

Repository
rL LLVM

Event Timeline

lhames created this revision.Jul 11 2019, 5:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2019, 5:58 PM

This is the fist time I’ve used the deprecation attribute, so I’ve just taken a stab at maintaining sanity here. To keep the tree building cleanly, since we still have tests and internal utilities using these classes, I have added the attribute to the constructors of classes being removed, and added new, non-warning constructors alongside, distinguished by a dummy argument at the start of the list. Adding ‘AcknowledgeORCv1Deprecation’ to the start of any call to a deprecated constructor thus removes the warning.

While I wait for any feedback on this I am going to work on the ORCv2 docs and add a section on transitioning from ORCv1. I hope to have all this in-tree before we branch for LLVM 9.0.

A lot of complexity and maintenance overhead sticks around in the code due to ORCv1. It complicates and slows down ORCv2 development. I agree that it's time to let go.

This is the fist time I’ve used the deprecation attribute, so I’ve just taken a stab at maintaining sanity here. To keep the tree building cleanly, since we still have tests and internal utilities using these classes, I have added the attribute to the constructors of classes being removed, and added new, non-warning constructors alongside, distinguished by a dummy argument at the start of the list. Adding ‘AcknowledgeORCv1Deprecation’ to the start of any call to a deprecated constructor thus removes the warning.

Interesting approach! The reason for it is also my question: Can we call something deprecated that we still use ourselves? The answer may vary for different parts of ORCv1.

OrcMCJITReplacement appears to be the pivotal point. It uses LegacyIRCompileLayer, LazyEmittingLayer and LegacyRTDyldLinkingLayer, but it's not marked deprecated itself. If they were removed, wouldn't OrcMCJITReplacement follow immediately?
What would that mean for the lit tests in llvm/test/ExecutionEngine/OrcMCJIT? We would probably discard a lot of them. Could some be moved to either orc-lazy or mcjit?
Should we deprecate lli's orc-mcjit mode?

And then I certainly won't miss:

  • layers LegacyObjectTransformLayer, RemoteObjectClient/ServerLayer, LegacyCompileOnDemandLayer, LegacyIRTransformLayer
  • utils like LegacyCtorDtorRunner or LegacyLocalCXXRuntimeOverrides

I hope to have all this in-tree before we branch for LLVM 9.0.

ORCv1 should be deprecated sooner rather than later. Though, a decision for the scope proposed here should come with a plan for OrcMCJITReplacement.

include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
277 ↗(On Diff #209386)

These look like clang-format artifacts.

... ORCv1 should be deprecated sooner rather than later. Though, a decision for the scope proposed here should come with a plan for OrcMCJITReplacement.

I'm going to deprecate OrcMCJITReplacement too. I'll have a patch up for that shortly. If there are enough people who need/want it we can re-instate it, but I'm hoping LLJIT will be as easy transition for most people.

lhames updated this revision to Diff 209960.Jul 15 2019, 2:37 PM
  • [ORC] Add deprecation warning for OrcMCJITReplacement
sgraenitz accepted this revision.Jul 16 2019, 10:24 AM
sgraenitz added inline comments.
tools/lli/lli.cpp
94 ↗(On Diff #209960)

Orc-based MCJIT replacement (deprecated)?

This revision is now accepted and ready to land.Jul 16 2019, 10:24 AM

What would that mean for the lit tests in llvm/test/ExecutionEngine/OrcMCJIT? We would probably discard a lot of them. Could some be moved to either orc-lazy or mcjit?

I checked on my machine and found that out of 78 tests in OrcMCJIT:

Not sure if this is a change we want now. What do you think?

Pardon 79. With the two patches applied:

$ cd /path/to/llvm-build-tree/test
$ python ../bin/llvm-lit -sv ExecutionEngine/OrcMCJIT/
Testing Time: 0.65s
  Expected Passes    : 77
  Expected Failures  : 2
lhames marked an inline comment as done.Jul 16 2019, 4:52 PM

That’s pretty encouraging. The main wrinkle is the remote tests: orc-lazy is currently running in-process only, so these tests aren’t properly testing remote-jitting support. I will see if I can get the remote-execution utility working with orc-lazy mode. That probably won’t make the branch cut-off, but hopefully can be done soon enough that we can cherry- pick before the 9.0 release.

Long term I would like to reconsider these tests. They were added in an ad-hoc fashion and many would now be better expressed as rtdyld or jitlink tests (neither of which were available at the time). I won’t have time to audit them before the release though, so perhaps the best option would be to pull them in under a new directory name (OrcLazy-LegacyTests?) in LLVM 9.0.

What do you think?

tools/lli/lli.cpp
94 ↗(On Diff #209960)

Good idea. I will update the patch in a moment.

lhames updated this revision to Diff 210220.Jul 16 2019, 4:54 PM
  • [ORC] Add a deprecation note to the lli -jit-kind=orc-mcjit option.

I’m going to land this. There may be fallout to example programs and other LLVM projects and want to try to shake that out before the 9.0 branch gets cut.

The main wrinkle is the remote tests: orc-lazy is currently running in-process only, so these tests aren’t properly testing remote-jitting support.
Long term I would like to reconsider these tests. They were added in an ad-hoc fashion and many would now be better expressed as rtdyld or jitlink tests (neither of which were available at the time).

Right. Maybe that should happen before the deprecated code is actually removed.

I will see if I can get the remote-execution utility working with orc-lazy mode. That probably won’t make the branch cut-off, but hopefully can be done soon enough that we can cherry-pick before the 9.0 release.

If that's a low-hanging fruit why not. Otherwise features might be better deferred to the next release?

[...] perhaps the best option would be to pull them in under a new directory name (OrcLazy-LegacyTests?) in LLVM 9.0.

Well, after all LLVM 9.0 has OrcMCJITReplacement. Thinking again, maybe it's best to leave these tests untouched for the current release and review them during the 10.0 time frame. Once all OrcMCJIT tests are sorted out, saying bye bye to ORCv1 sounds reasonable to me.

I’m going to land this. There may be fallout to example programs and other LLVM projects and want to try to shake that out before the 9.0 branch gets cut.

Agree, thanks for working on this!

This revision was automatically updated to reflect the committed changes.

@lhames We have deprecation warnings (and Werror failures) on the buildbots due to this change, are you in a position to get these fixed prior to branching off 9.00?