Page MenuHomePhabricator

[lld-macho] Run ObjCContractPass during LTO

Authored by int3 on Jan 12 2021, 12:45 PM.



Run the ObjCARCContractPass during LTO. The legacy LTO backend (under
LTO/ThinLTOCodeGenerator.cpp) already does this; this diff just adds that
behavior to the new LTO backend. Without that pass, the objc.clang.arc.use
intrinsic will get passed to the instruction selector, which doesn't know how to
handle it.

In order to test both the new and old pass managers, I've also added support for
the --[no-]lto-legacy-pass-manager flags.

P.S. Not sure if the ordering of the pass within the pipeline matters...

Diff Detail

Event Timeline

int3 created this revision.Jan 12 2021, 12:45 PM
int3 requested review of this revision.Jan 12 2021, 12:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2021, 12:45 PM
lanza added a comment.Jan 12 2021, 3:57 PM

This goes right before codegen in ThinLTOCodeGenerator. TM.addPassesToEmitFile is the entry point to the llvm backend. So the legacy implementation has this pass ran immediately after the front-end is finished and right before the backend starts. I imagine doing the same here makes the most sense.

int3 updated this revision to Diff 316410.Jan 13 2021, 8:30 AM

add passes at the end of the pipeline

lanza added a comment.Jan 13 2021, 6:41 PM



Sorry, to be clearer I think around here is the analogous point for the LTO.cpp pipeline. It directly precedes addPassesToEmitFile in the legacy impl.

lanza added inline comments.Jan 13 2021, 6:42 PM

codegen and optimizations aren't directly coupled. You can codegen without optimizing and vice versa and run optimizations multiple times in the legacy impl. So this pass should stay coupled to codegen.

fhahn added a subscriber: fhahn.Jan 14 2021, 12:49 AM
fhahn added inline comments.

The reason this is not added to all backend pipelines is (presumably) that we do not want to run this unnecessarily on platforms that do not support ObjC. I think we should add a new flag to lto::Config, so users can request it. But as I mentioned below, I think it would be better to do it at the beginning of the codegen pass pipeline.


Yes, you don't necessarily have to run the optimize stage, but we still need to ensure the pass is run before codegen. So I think here would be a suitable place to just add the pass to CodeGenPasses (LTOCodeGenerator.cpp runs it just before calling splitCodeGen

int3 added inline comments.Jan 15 2021, 9:07 AM

Isn't it technically possible for every platform to support ObjC, e.g. with GNUstep?

int3 added inline comments.Jan 15 2021, 9:11 AM

(But I suppose it's a good idea not to make folks pay for edge cases that are in practice never used)

fhahn added inline comments.Jan 15 2021, 9:36 AM

Isn't it technically possible for every platform to support ObjC, e.g. with GNUstep?

I think all the matters in this case are frontends that emit LLVM IR I think. Not sure if Clang + ObjC is actually usable on non-Darwin platforms in practice.

In any case, I think it would be worth making it opt-in to start with. And if you are looking for enabling it unconditionally, it would probably be good as a separate patch and with a concrete motivation behind it.

int3 updated this revision to Diff 317008.Jan 15 2021, 10:03 AM

run pass right before codegen via a config hook

int3 added a comment.Jan 15 2021, 10:08 AM

Since we're only running the ObjCContractPass via the one pass manager in codegen, there's actually no real need to add the --[no-]lto-legacy-pass-manager flags in this diff. But it probably doesn't hurt to leave it in, along with the test, just to make sure that any future migrations to the non-legacy PM don't break behavior

47 ↗(On Diff #317009)

I initially tried making this a vector of Pass *, but it appears that creating the ObjCContractPass too early results in an error later:

Assertion failed: (!Resolver && "Resolver is already set"), function setResolver, file /Users/jezng/src/llvm-project/llvm/lib/IR/Pass.cpp

Using this hook to delay initialization seems to work, though I'm not sure if it's the right solution.

Harbormaster completed remote builds in B85382: Diff 317009.
fhahn added inline comments.Jan 16 2021, 8:53 AM
47 ↗(On Diff #317009)

Can you a doc-comment for PreCodeGenPassesHook?


Can we just keep the name of the pass manager as CodeGenPasses?

int3 added inline comments.Jan 19 2021, 10:51 AM
47 ↗(On Diff #317009)

For sure, was planning to do that. Just wanted to make sure that using a hook to delay initialization was the right approach here...


I'd thought that since we are now including pre-codegen passes in it, the old name was a bit of a misnomer... but I don't feel strongly about it

int3 updated this revision to Diff 317687.Jan 19 2021, 2:04 PM
int3 marked 2 inline comments as done.

address comments

fhahn accepted this revision.Jan 20 2021, 4:06 AM

LGTM, thanks!

47 ↗(On Diff #317687)

nit: /// for doc-comment

This revision is now accepted and ready to land.Jan 20 2021, 4:06 AM
This revision was landed with ongoing or failed builds.Jan 20 2021, 11:21 AM
This revision was automatically updated to reflect the committed changes.
int3 marked an inline comment as done.