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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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.
tools/lli/lli.cpp | ||
---|---|---|
94 ↗ | (On Diff #209960) | Orc-based MCJIT replacement (deprecated)? |
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:
- 72 work with orc-lazy https://reviews.llvm.org/differential/diff/210161/
- only 6 are failing with orc-lazy but succeed with mcjit https://reviews.llvm.org/differential/diff/210162/
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
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. |
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.
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!
@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?