Page MenuHomePhabricator

Replace addEarlyAsPossiblePasses callback with adjustPassManager
ClosedPublic

Authored by rampitec on Jan 4 2017, 4:55 PM.

Details

Summary

This change introduces adjustPassManager target callback giving a target an opportunity to tweak PassManagerBuilder before pass managers are populated.

This generalizes and replaces addEarlyAsPossiblePasses target callback. In particular that can be used to add custom passes to extension points other than EP_EarlyAsPossible.

If approved the similar change to clang's BackendUtil.cpp will be created.

Diff Detail

Repository
rL LLVM

Event Timeline

rampitec updated this revision to Diff 83167.Jan 4 2017, 4:55 PM
rampitec retitled this revision from to Replace addEarlyAsPossiblePasses callback with adjustPassManager.
rampitec updated this object.
rampitec added a reviewer: jlebar.
rampitec set the repository for this revision to rL LLVM.
rampitec added a subscriber: llvm-commits.
jlebar edited edge metadata.EditedJan 4 2017, 5:02 PM
jlebar added a subscriber: chandlerc.

This seems like a reasonable thing to do to me, but I'd like @chandlerc to sign off on it.

Note that it still doesn't quite get you module-level early-as-possible passes, because you have to use the EarlyModule extension point. But at least we don't have to add a new interface to the TM to let it muck with that extension point...

If approved the similar change to clang's BackendUtil.cpp will be created.

If you want you can use the monorepo to land these two patches as a single unit, http://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo

include/llvm/Target/TargetMachine.h
218 ↗(On Diff #83167)

Perhaps something like

Allow the target to modify the pass manager, e.g. by calling PassManagerBuilder::addExtension.

jlebar accepted this revision.Jan 4 2017, 5:02 PM
jlebar added a reviewer: chandlerc.
jlebar edited edge metadata.

Note that it still doesn't quite get you module-level early-as-possible passes, because you have to use the EarlyModule extension point. But at least we don't have to add a new interface to the TM to let it muck with that extension point...

Yes, I can bear with it. I think if we really need a module pass working earlier, that would mean creating a new extension point. This seems to be more orthogonal.

rampitec updated this revision to Diff 83171.Jan 4 2017, 5:53 PM
rampitec edited edge metadata.

Changed comment as proposed.

rampitec marked an inline comment as done.Jan 4 2017, 5:53 PM

The older version of the change which lead to this version os here: D28031

Ping. @chandlerc, what do you think?

Ping.
In D28694 @chandlerc has proposed exactly this solution: "What about instead you call a method on TargetMachine with the builder that can add any extensions it wants." It looks like the principle should be acceptable.

Since this review has been accepted, and Chandler suggested this exact thing in another review, I think this review is good to go. Can you commit it?

include/llvm/Target/TargetMachine.h
23 ↗(On Diff #83171)

You don't have to include this file, since you only have references to the PassManagerBuilder class. You can add a forward declaration of PassManagerBuilder below in "namespace llvm".

Since this review has been accepted, and Chandler suggested this exact thing in another review, I think this review is good to go. Can you commit it?

I guess I still need an explicit approval from @chandlerc since he is a blocking reviewer.

rampitec marked an inline comment as done.Jan 25 2017, 9:34 AM
rampitec updated this revision to Diff 85767.Jan 25 2017, 9:36 AM

Replaced PassManagerBuilder.h include with forward class declaration in TargetMachine.h.

chandlerc accepted this revision.Jan 25 2017, 6:14 PM

Thanks this looks really nice! LGTM!

This revision is now accepted and ready to land.Jan 25 2017, 6:14 PM
This revision was automatically updated to reflect the committed changes.

This seems to have broken LLVM builds on Windows if either the NVPTX or Hexagon targets are enabled. You end up with many linker errors of the form

LLVMNVPTXCodeGen.lib(NVPTXTargetMachine.obj) : error LNK2019: unresolved external symbol "public: void __cdecl llvm::PassManagerBuilder::addExtension(enum llvm::PassManagerBuilder::ExtensionPointTy,class std::function<void __cdecl(class llvm::PassManagerBuilder const &,class llvm::legacy::PassManagerBase &)>)" (?addExtension@PassManagerBuilder@llvm@@QEAAXW4ExtensionPointTy@12@V?$function@$$A6AXAEBVPassManagerBuilder@llvm@@AEAVPassManagerBase@legacy@2@@Z@std@@@Z) referenced in function "public: virtual void __cdecl llvm::NVPTXTargetMachine::adjustPassManager(class llvm::PassManagerBuilder &)" (?adjustPassManager@NVPTXTargetMachine@llvm@@UEAAXAEAVPassManagerBuilder@2@@Z)

(Ditto for Hexagon)

Breaks build on other platforms too, for example: http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/1928/steps/build%20stage%201/logs/stdio

Broke my local build too, will try again later.

I suspect the issue is not windows versus other operating systems but statically-linked versus dynamically-linked. Usually we just need to update an LLVMBuild.txt file somewhere.

I'll try to look at this soon if someone doesn't get to it first.

This seems to have broken LLVM builds on Windows if either the NVPTX or Hexagon targets are enabled. You end up with many linker errors of the form

Breaks build on other platforms too, for example: http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/1928/steps/build%20stage%201/logs/stdio

Broke my local build too, will try again later.

I have just built it clean on Windows with VS2015, including clang. I have also built it several times since on Linux.
I'm looking into the supplied log, and I cannot see NVPTXTargetMachine.cpp is being rebuilt, while it should. I had to do two commits, one to llvm and one to clang, so maybe something stale in the build directory...
Can you please try a clean build?

rampitec added a comment.EditedJan 26 2017, 6:28 PM

I suspect the issue is not windows versus other operating systems but statically-linked versus dynamically-linked. Usually we just need to update an LLVMBuild.txt file somewhere.

I'll try to look at this soon if someone doesn't get to it first.

Oh, You might be right. I think we need to add IPO to the required_libraries for both backends. Do you mind me adding it?

UPD: Looks like it is just added.

Looks like this was fixed for NVPTX in r293256, Hexagon earlier. Can confirm resolves the issue I was encountering, thanks!

(And I think you're right, it only seems to have occurred in the shared library variants)

But now undefined reference in AMDGPU haha, but not relating to PassManagerBuilder :).

Looks like this was fixed for NVPTX in r293256, Hexagon earlier. Can confirm resolves the issue I was encountering, thanks!

(And I think you're right, it only seems to have occurred in the shared library variants)

But now undefined reference in AMDGPU haha, but not relating to PassManagerBuilder :).

Thanks and sorry I did not notice it right away. For some reason my builds do work, just as Linux shared build I did right now. Do you still see error building AMDGPU or is it already fixed as well?

Looks like this was fixed for NVPTX in r293256, Hexagon earlier. Can confirm resolves the issue I was encountering, thanks!

(And I think you're right, it only seems to have occurred in the shared library variants)

But now undefined reference in AMDGPU haha, but not relating to PassManagerBuilder :).

Thanks and sorry I did not notice it right away. For some reason my builds do work, just as Linux shared build I did right now. Do you still see error building AMDGPU or is it already fixed as well?

No worries :). Recently GlobalISel started to defaulting to 'on' which exposes the (conditionally) missing dependency... I committed a fix in r293387.

re:that change, I didn't see any way to make LLVMBuild.txt contain conditional dependencies so it might overlink when GlobalISel is disabled? But it's what the other targets do so hopefully that's not a problem.

As for why you didn't see it, honestly my build env is sufficiently strange that I'd be happy to blame it on that-- among other things I'm intentionally forcing stricter linking behavior which would the different results.

Also, if you explicitly disable GlobalISel or maybe if an update+rebuild retained the previous default value of "off"? Dunno :).

Anyway, should be fixed now, thanks!