Replace addEarlyAsPossiblePasses callback with adjustPassManager

[rampitec]

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.

[jlebar]

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–220

Perhaps something like

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

[rampitec]

In D28336#636303, @jlebar wrote:

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]

Changed comment as proposed.

[rampitec]

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

[rampitec]

Ping. @chandlerc, what do you think?

[rampitec]

Ping

[rampitec]

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.

[kparzysz]

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

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".

[rampitec]

In D28336#656341, @kparzysz wrote:

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]

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

[chandlerc]

Thanks this looks really nice! LGTM!

[steven-johnson]

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)

[dtzWill]

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.

[jlebar]

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.

[rampitec]

In D28336#658237, @steven-johnson wrote:

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

In D28336#658340, @dtzWill wrote:

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]

In D28336#658349, @jlebar wrote:

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.

[dtzWill]

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 :).

[rampitec]

In D28336#658696, @dtzWill wrote:

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?

[dtzWill]

In D28336#658899, @rampitec wrote:

In D28336#658696, @dtzWill wrote:

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!

include/llvm/Target/TargetMachine.h

 #define LLVM_TARGET_TARGETMACHINE_H
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/CodeGen.h"
 #include "llvm/Target/TargetOptions.h"
+#include "llvm/Transforms/IPO/PassManagerBuilder.h"
 #include <cassert>
 #include <string>
 
 namespace llvm {
 
 class InstrItineraryData;
 class GlobalValue;
 class Mangler;
public:
 
   /// \brief Get a \c TargetIRAnalysis appropriate for the target.
   ///
   /// This is used to construct the new pass manager's target IR analysis pass,
   /// set up appropriately for this target machine. Even the old pass manager
   /// uses this to answer queries about the IR.
   virtual TargetIRAnalysis getTargetIRAnalysis();
 
-  /// Add target-specific function passes that should be run as early as
-  /// possible in the optimization pipeline.  Most TargetMachines have no such
-  /// passes.
-  virtual void addEarlyAsPossiblePasses(PassManagerBase &) {}
+  /// Allow the target to modify the pass manager, e.g. by calling
+  /// PassManagerBuilder::addExtension.
+  virtual void adjustPassManager(PassManagerBuilder &) {}
 
   /// These enums are meant to be passed into addPassesToEmitFile to indicate
   /// what type of file to emit, and returned by it to indicate what type of
   /// file could actually be made.
   enum CodeGenFileType {
     CGFT_AssemblyFile,
     CGFT_ObjectFile,
     CGFT_Null         // Do not emit any output.

lib/Target/AMDGPU/AMDGPUTargetMachine.h

public:
   const AMDGPUIntrinsicInfo *getIntrinsicInfo() const override {
     return &IntrinsicInfo;
   }
   TargetIRAnalysis getTargetIRAnalysis() override;
 
   TargetLoweringObjectFile *getObjFileLowering() const override {
     return TLOF.get();
   }
-  void addEarlyAsPossiblePasses(PassManagerBase &PM) override;
+
+  void adjustPassManager(PassManagerBuilder &) override;
 };
 
 //===----------------------------------------------------------------------===//
 // R600 Target Machine (R600 -> Cayman)
 //===----------------------------------------------------------------------===//
 
 class R600TargetMachine final : public AMDGPUTargetMachine {
 private:

lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

 StringRef AMDGPUTargetMachine::getFeatureString(const Function &F) const {
   Attribute FSAttr = F.getFnAttribute("target-features");
 
   return FSAttr.hasAttribute(Attribute::None) ?
     getTargetFeatureString() :
     FSAttr.getValueAsString();
 }
 
-void AMDGPUTargetMachine::addEarlyAsPossiblePasses(PassManagerBase &PM) {
+void AMDGPUTargetMachine::adjustPassManager(PassManagerBuilder &Builder) {
+  Builder.addExtension(
+    PassManagerBuilder::EP_EarlyAsPossible,
+    [&](const PassManagerBuilder &, legacy::PassManagerBase &PM) {
       PM.add(createAMDGPUUnifyMetadataPass());
+    });
 }
 
 //===----------------------------------------------------------------------===//
 // R600 Target Machine (R600 -> Cayman)
 //===----------------------------------------------------------------------===//
 
 R600TargetMachine::R600TargetMachine(const Target &T, const Triple &TT,
                                      StringRef CPU, StringRef FS,

lib/Target/NVPTX/NVPTXTargetMachine.h

public:
   bool addPassesToEmitMC(PassManagerBase &, MCContext *&, raw_pwrite_stream &,
                          bool = true) override {
     return true;
   }
   TargetLoweringObjectFile *getObjFileLowering() const override {
     return TLOF.get();
   }
 
-  void addEarlyAsPossiblePasses(PassManagerBase &PM) override;
+  void adjustPassManager(PassManagerBuilder &) override;
+
   TargetIRAnalysis getTargetIRAnalysis() override;
 
 }; // NVPTXTargetMachine.
 
 class NVPTXTargetMachine32 : public NVPTXTargetMachine {
   virtual void anchor();
 public:
   NVPTXTargetMachine32(const Target &T, const Triple &TT, StringRef CPU,

lib/Target/NVPTX/NVPTXTargetMachine.cpp

private:
   void addStraightLineScalarOptimizationPasses();
 };
 } // end anonymous namespace
 
 TargetPassConfig *NVPTXTargetMachine::createPassConfig(PassManagerBase &PM) {
   return new NVPTXPassConfig(this, PM);
 }
 
-void NVPTXTargetMachine::addEarlyAsPossiblePasses(PassManagerBase &PM) {
+void NVPTXTargetMachine::adjustPassManager(PassManagerBuilder &Builder) {
+  Builder.addExtension(
+    PassManagerBuilder::EP_EarlyAsPossible,
+    [&](const PassManagerBuilder &, legacy::PassManagerBase &PM) {
       PM.add(createNVVMReflectPass());
       PM.add(createNVVMIntrRangePass(Subtarget.getSmVersion()));
+    });
 }
 
 TargetIRAnalysis NVPTXTargetMachine::getTargetIRAnalysis() {
   return TargetIRAnalysis([this](const Function &F) {
     return TargetTransformInfo(NVPTXTTIImpl(this, F));
   });
 }
 

tools/opt/opt.cpp

static void AddOptimizationPasses(legacy::PassManagerBase &MPM,
   // If option wasn't forced via cmd line (-vectorize-loops, -loop-vectorize)
   else if (!Builder.LoopVectorize)
     Builder.LoopVectorize = OptLevel > 1 && SizeLevel < 2;
 
   // When #pragma vectorize is on for SLP, do the same as above
   Builder.SLPVectorize =
       DisableSLPVectorization ? false : OptLevel > 1 && SizeLevel < 2;
 
-  // Add target-specific passes that need to run as early as possible.
   if (TM)
-    Builder.addExtension(
-        PassManagerBuilder::EP_EarlyAsPossible,
-        [&](const PassManagerBuilder &, legacy::PassManagerBase &PM) {
-          TM->addEarlyAsPossiblePasses(PM);
-        });
+    TM->adjustPassManager(Builder);
 
   if (Coroutines)
     addCoroutinePassesToExtensionPoints(Builder);
 
   Builder.populateFunctionPassManager(FPM);
   Builder.populateModulePassManager(MPM);
 }