Allow target to specify early module passes

[rampitec]

Summary

The addEarlyAsPossiblePasses target callback can only accept function passes. This patch extends the functionality to support module passes as well.

Default argument PMT_FunctionPassManager is added until clang's BackendUtil.cpp is updated as well, then it can be removed.

[jlebar]

I strongly approve of the principle of letting us add early-as-possible module passes, but I am not sure about the API here. It seems kind of clunky that we have to check if (PMT != PMT_FunctionPassManager), and that when building the PM we have to call addExtension twice.

Could we instead just make the addEarlyAsPossiblePasses API take a module pass manager? It would be the responsibility of the target to instantiate a function pass manager and add it to the MPM. This way our solution is fully general -- if we decide we want (say) early-as-possible CGSCC passes, we don't have to do anything special to make that work.

Separately, can we split the AMDGPU changes into a separate patch?

[rampitec]

In D28031#631812, @jlebar wrote:

Could we instead just make the addEarlyAsPossiblePasses API take a module pass manager? It would be the responsibility of the target to instantiate a function pass manager and add it to the MPM. This way our solution is fully general -- if we decide we want (say) early-as-possible CGSCC passes, we don't have to do anything special to make that work.

Isn't it too heavy? We would need to create a new FPM, when we already have one, and it is more code for target. FunctionPassManager requires Module as an argument to ctor, so target will need to create a module pass, which will then instantiate an FPM and run it. As far as I understand that would also happen not exactly at the same point of time as now, i.e. later than possible.

How about changing API to pass a bulder object into addEarlyAsPossiblePasses instead of PassManagerBase? In this case target could just call Builder.addExtension() itself with any extension type it needs and as much times it needs.

[jlebar]

Isn't it too heavy? We would need to create a new FPM, when we already have one, and it is more code for target.

This is not a commonly-used API, so I am not too concerned with it taking (say) 20 lines instead of 5 lines to add an early-as-possible function pass. If this is really a problem, we can provide a helper method that accepts a Function pass (or an ArrayRef of function passes, if you like), but I am not sure that will be necessary.

FunctionPassManager requires Module as an argument to ctor, so target will need to create a module pass, which will then instantiate an FPM and run it.

Right.

As far as I understand that would also happen not exactly at the same point of time as now, i.e. later than possible.

The way you're imagining this would work is we would use the existing ModuleOptimizerEarly extension point? I agree that would not be as early as possible. I was suggesting instead that we change the existing early-as-possible extension point to run a module pass instead of a function pass. In this way, it would run at the same point in time as it does today.

How about changing API to pass a bulder object into addEarlyAsPossiblePasses instead of PassManagerBase? In this case target could just call Builder.addExtension() itself with any extension type it needs and as much times it needs.

This seems too general: Given a PassManagerBuilder object, you can do anything, not just add early-as-possible passes.

[rampitec]

In D28031#634230, @jlebar wrote:

This seems too general: Given a PassManagerBuilder object, you can do anything, not just add early-as-possible passes.

I personally do not see a big issue with the interface being too general, but I see your point. Given your proposed approach will run early module passes earlier than now I also think that is a benefit. I will try to code it and see how it goes.

[rampitec]

In D28031#634359, @rampitec wrote:

In D28031#634230, @jlebar wrote:

This seems too general: Given a PassManagerBuilder object, you can do anything, not just add early-as-possible passes.

I personally do not see a big issue with the interface being too general, but I see your point. Given your proposed approach will run early module passes earlier than now I also think that is a benefit. I will try to code it and see how it goes.

Here are two problems:

1. While addEarlyAsPossiblePasses is indeed rarely used API, the EP_EarlyAsPossible itself is not so rare. I would need to convert to module passes CoroEarly and AddDiscriminatorsLegacyPass in addition to those from NVPTX and AMDGPU in case if I want to use EP_EarlyAsPossible. An alternative is to add a new ExtensionPoint to duplicate EP_ModuleOptimizerEarly behavior, except making it running even earlier. EP_ModuleOptimizerReallyEarly ;)
2. This is worse. If I need to run module passes where we run EarlyAsPossible passes, I would need to have a module pass manager at this place. Builder.populateFunctionPassManager only takes a function pass manager. So I do need yet another one module pass manager created and run either from opt.cpp/BackendUtil.cpp or in FunctionPassManagerImpl::run(). All of that just to let early function passes create their own twice-nested function pass managers.

All this seems to be really unneeded overhead to me. Maybe still consider passing PassManagerBuilder to a target, or just add addEarlyModulePasses callback? The latter could create its own SCC, Function or any other pass manager if so need.

Then if you are concerned about passing a PassManagerBuilder to be too general, it might make sense to create a new target callback with an appropriate general name, like adjustPassManager to reflect the general nature of the callback. In fact it can supersede current addEarlyModulePasses, which then could be dropped.

[rampitec]

What I can also do is to stop using extensions at all. I.e. write this into opt.cpp and BackendUtil.cpp:

   if (OptLevelO3)
     AddOptimizationPasses(Passes, *FPasses, TM.get(), 3, 0);

+  // Add target-specific passes that need to run as early as possible.
+  if (TM) {
+    legacy::PassManager EarlyPasses;
+    TM->addEarlyAsPossiblePasses(EarlyPasses);
+    EarlyPasses.run(*M);
+  }
+
   if (FPasses) {
     FPasses->doInitialization();

[rampitec]

I have modified the patch to always use module pass manager, as discussed.
Justin, I could extract new pass in a separate file and add INITIALIZE_PASS if you wish, but I'm really not sure you will like the implementation when you see it.
Of course the same change will be needed in the BackendUtil.cpp.
Sorry, I did not split off AMDGPU changes, as I would then need to create all the same wrapper for function pass.
Also note, to make this working I had to patch NVVMIntrRange.cpp. The problem is NVPTXSubtarget::SmVersion is only initialized to value 20 and never modified. The pass expects value from -nvvm-intr-range-sm, and whenever the option or NVPTXSubtarget::SmVersion would take precedence really depends on the initialization order. This is just a w/a for the problem exposed by the change.

[jlebar]

I'd like Chandler to weigh in here on the approach. Ideally frontends would not have to do anything to run the early-as-possible passes -- it's pretty annoying that we cannot rely on them being run. But we have in this patch also isn't quite what I had in mind. Let me try to spin a patch.

[rampitec]

In D28031#636128, @jlebar wrote:

I'd like Chandler to weigh in here on the approach. Ideally frontends would not have to do anything to run the early-as-possible passes -- it's pretty annoying that we cannot rely on them being run. But we have in this patch also isn't quite what I had in mind. Let me try to spin a patch.

I'm leaning towards giving targets a hook to accept the PassManagerBuilder and adjust as they want it.

[jlebar]

I'm leaning towards giving targets a hook to accept the PassManagerBuilder and adjust as they want it.

Me too. I don't think it should be called "early as possible", but in principle I think I agree.

[jlebar]

I'm leaning towards giving targets a hook to accept the PassManagerBuilder and adjust as they want it.

Me too. I don't think it should be called "early as possible", but in principle I think I agree.

(I'll let you spin another patch with that approach.)

[rampitec]

In D28031#636151, @jlebar wrote:

I'm leaning towards giving targets a hook to accept the PassManagerBuilder and adjust as they want it.

Me too. I don't think it should be called "early as possible", but in principle I think I agree.

(I'll let you spin another patch with that approach.)

Agree.

lib/Target/AMDGPU/AMDGPU.h

 ModulePass *createAMDGPUOpenCLImageTypeLoweringPass();
 FunctionPass *createAMDGPUAnnotateUniformValues();
 
-FunctionPass* createAMDGPUUnifyMetadataPass();
+ModulePass* createAMDGPUUnifyMetadataPass();
 void initializeAMDGPUUnifyMetadataPass(PassRegistry&);
 extern char &AMDGPUUnifyMetadataID;
 

lib/Target/AMDGPU/AMDGPUUnifyMetadata.cpp

   }
 
   /// \brief Unify multiple OpenCL metadata due to linking.
-  class AMDGPUUnifyMetadata : public FunctionPass {
+  class AMDGPUUnifyMetadata : public ModulePass {
   public:
     static char ID;
-    explicit AMDGPUUnifyMetadata() : FunctionPass(ID) {};
+    explicit AMDGPUUnifyMetadata() : ModulePass(ID) {};
 
   private:
-    // This should really be a module pass but we have to run it as early
-    // as possible, so given function passes are executed first and
-    // TargetMachine::addEarlyAsPossiblePasses() expects only function passes
-    // it has to be a function pass.
     virtual bool runOnModule(Module &M);
 
-    // \todo: Convert to a module pass.
-    virtual bool runOnFunction(Function &F);
-
     /// \brief Unify version metadata.
     /// \return true if changes are made.
     /// Assume the named metadata has operands each of which is a pair of

                 "Unify multiple OpenCL metadata due to linking",
                 false, false)
 
-FunctionPass* llvm::createAMDGPUUnifyMetadataPass() {
+ModulePass* llvm::createAMDGPUUnifyMetadataPass() {
   return new AMDGPUUnifyMetadata();
 }
 

 
   return Changed;
 }
-
-bool AMDGPUUnifyMetadata::runOnFunction(Function &F) {
-  return runOnModule(*F.getParent());
-}

lib/Target/NVPTX/NVPTXTargetMachine.cpp

   // Add passes that perform straight-line scalar optimizations.
   void addStraightLineScalarOptimizationPasses();
 };
+
+class NVPTXEarlyAsPossibleFunctionWrapperPass : public ModulePass {
+public:
+  static char ID;
+  NVPTXEarlyAsPossibleFunctionWrapperPass(NVPTXTargetMachine &TM_) :
+    ModulePass(ID), TM(TM_) {}
+
+  bool runOnModule(Module &M) override;
+
+private:
+  NVPTXTargetMachine &TM;
+};
 } // end anonymous namespace
 
 TargetPassConfig *NVPTXTargetMachine::createPassConfig(PassManagerBase &PM) {
   return new NVPTXPassConfig(this, PM);
 }
 
+char NVPTXEarlyAsPossibleFunctionWrapperPass::ID = 0;
+
+bool NVPTXEarlyAsPossibleFunctionWrapperPass::runOnModule(Module &M) {
+  legacy::FunctionPassManager *FPM = new legacy::FunctionPassManager(&M);
+
+  FPM->add(createNVVMReflectPass());
+  FPM->add(createNVVMIntrRangePass(TM.getSubtargetImpl()->getSmVersion()));
+
+  FPM->doInitialization();
+  for (auto &F : M.functions())
+    FPM->run(F);
+  FPM->doFinalization();
+  delete FPM;
+
+  return true;
+}
+
 void NVPTXTargetMachine::addEarlyAsPossiblePasses(PassManagerBase &PM) {
-  PM.add(createNVVMReflectPass());
-  PM.add(createNVVMIntrRangePass(Subtarget.getSmVersion()));
+  PM.add(new NVPTXEarlyAsPossibleFunctionWrapperPass(*this));
 }
 
 TargetIRAnalysis NVPTXTargetMachine::getTargetIRAnalysis() {

lib/Target/NVPTX/NVVMIntrRange.cpp

 }
 
 FunctionPass *llvm::createNVVMIntrRangePass(unsigned int SmVersion) {
-  return new NVVMIntrRange(SmVersion);
+  return new NVVMIntrRange((NVVMIntrRangeSM.getNumOccurrences() > 0) ?
+                             NVVMIntrRangeSM : SmVersion);
 }
 
 char NVVMIntrRange::ID = 0;

test/CodeGen/AMDGPU/unify-metadata.ll

 ; ALL-DAG: ![[USED_EXT_1]] = !{!"cl_khr_fp16"}
 ; ALL-DAG: ![[USED_EXT_2]] = !{!"cl_doubles"}
 
-define void @test() {
-   ret void
-}
-
 !opencl.ocl.version = !{!1, !0, !0, !0}
 !llvm.ident = !{!2, !2, !2, !2, !6}
 !opencl.used.extensions = !{!3, !3, !4, !5}

tools/opt/opt.cpp

   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);
-        });
-
   if (Coroutines)
     addCoroutinePassesToExtensionPoints(Builder);
 

   if (OptLevelO3)
     AddOptimizationPasses(Passes, *FPasses, TM.get(), 3, 0);
 
+  // Add target-specific passes that need to run as early as possible.
+  if (TM) {
+    legacy::PassManager EarlyPasses;
+    TM->addEarlyAsPossiblePasses(EarlyPasses);
+    EarlyPasses.run(*M);
+  }
+
   if (FPasses) {
     FPasses->doInitialization();
     for (Function &F : *M)