[RISCV] Move class RISCVPassConfig declaration to RISCVTargetMachine.h for downstream. NFC

[Jim]

Summary

We have special need to create a new class inherited from RISCVPassConfig
for just overriding some functions in RISCVPassConfig in our downstream.
And return our new customized class instead.

It couldn't achieve this if class RISCVPassConfig declaration is in RISCVTargetMachine.cpp.
So, move it to RISCVTargetMachine.h.

[craig.topper]

Can you clarify what *why* this helps downstream?

[Jim]

clang-format

[Jim]

Add commit log

[Jim]

In D120695#3350370, @craig.topper wrote:

Can you clarify what *why* this helps downstream?

Add summary.

[craig.topper]

Isn’t the function that returns the class in this cpp file? Why are you able to change that function in your downstream but can’t add your new class to the cpp in your downstream?

[Jim]

In D120695#3350392, @craig.topper wrote:

Isn’t the function that returns the class in this cpp file? Why are you able to change that function in your downstream but can’t add your new class to the cpp in your downstream?

The return function in this cpp is the only place needed to be changed to return new class.
Also add a new #include to include new class header file in this cpp.

In my mind, this two places are hard to conflict with upstream or easily to fix the conflict if upgrade our downstream.

[jrtc27]

Inheriting from the pass doesn't make sense, just change the implementations. The TargetPassConfig has access to the TM for a reason. As someone with significant downstream changes I am sympathetic to changing upstream code to be more accommodating, but this change just seems motivated by an arbitrary downstream decision that goes against the intent of LLVM's structure just to avoid the occasional merge conflict (RISCVPassConfig hardly changes much).

[jrtc27]

Note that git grep ' : public TargetPassConfig' llvm/lib/Target turns up 21 results, exactly one for each backend, and every single one lives in the backend's TargetMachine.cpp. So moving it to a header, or adding a second one, would go against the convention of every single backend in tree.

[Jim]

In D120695#3351508, @jrtc27 wrote:

Inheriting from the pass doesn't make sense, just change the implementations. The TargetPassConfig has access to the TM for a reason. As someone with significant downstream changes I am sympathetic to changing upstream code to be more accommodating, but this change just seems motivated by an arbitrary downstream decision that goes against the intent of LLVM's structure just to avoid the occasional merge conflict (RISCVPassConfig hardly changes much).

We create a new class inherited from RISCVPassConfig and make some functions overridden to fit our need. And return our class instead of RISCVPassConfig.
Lot of behaviors between our class and RISCVPassConfig are the same. It doesn't have any change to access TM.
It is not only to avoid the occasional merge conflict. If the class RISCVPassConfig moved to RISCVTargetMachine.h, could let the dowstream reuse RISCVPassConfig in the elegant way (just include header file and inherit it, not touch upstream file much).

[Jim]

In D120695#3351520, @jrtc27 wrote:

Note that git grep ' : public TargetPassConfig' llvm/lib/Target turns up 21 results, exactly one for each backend, and every single one lives in the backend's TargetMachine.cpp. So moving it to a header, or adding a second one, would go against the convention of every single backend in tree.

Yes, every backend only have one class inherited from TargetPassConfig. But the downstream class is inherited from RISCVPassConfig. Doesn't have second one public TargetPassConfig.
And I refer from AMDGPU target, it seems AMDGPUPassConfig is declared in AMDGPU/AMDGPUTargetMachine.h not AMDGPU/AMDGPUTargetMachine.cpp.

[craig.topper]

Creating a new class seems fragile. Does your class call the RISCVPassConfig version of the methods you override? What if your class overrides a method that RISCVPassConfig doesn't override today, but does in the future. Will your class call the newly added override when its done with your override?

[Jim]

In D120695#3353293, @craig.topper wrote:

Creating a new class seems fragile. Does your class call the RISCVPassConfig version of the methods you override? What if your class overrides a method that RISCVPassConfig doesn't override today, but does in the future. Will your class call the newly added override when its done with your override?

Our class would call and reuse the RISCVPassConfig version of the methods in most of situations.
If upgrade the base upstream, we have to take care every the newly added override function in the parent class if it is already existed in our class.
If the implementations are the same, remove duplicate one from our class. Or trying to reuse the RISCVPassConfig version depending on the need.

[Jim]

It not just happen in RISCVPassConfig. If we inherit other class such as RISCVISelDAGToDAG.h and RISCVISelLowering.h, it still need to deal with the newly added override function issue.

[jrtc27]

Why do you need to derive from any of them. Just modify them. It's so much more painful to do it the way you're suggesting. I say this as a downstream that makes significant changes to instruction selection and adds totally new ABIs.

llvm/lib/Target/RISCV/RISCVTargetMachine.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_RISCV_RISCVTARGETMACHINE_H
 #define LLVM_LIB_TARGET_RISCV_RISCVTARGETMACHINE_H
 
 #include "MCTargetDesc/RISCVMCTargetDesc.h"
 #include "RISCVSubtarget.h"
+#include "llvm/CodeGen/Passes.h"
 #include "llvm/CodeGen/SelectionDAGTargetInfo.h"
+#include "llvm/CodeGen/TargetPassConfig.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/Target/TargetMachine.h"
 
 namespace llvm {
 class RISCVTargetMachine : public LLVMTargetMachine {
   std::unique_ptr<TargetLoweringObjectFile> TLOF;
   mutable StringMap<std::unique_ptr<RISCVSubtarget>> SubtargetMap;
 
TargetLoweringObjectFile *getObjFileLowering() const override {
     return TLOF.get();
   }
 
   TargetTransformInfo getTargetTransformInfo(const Function &F) const override;
 
   virtual bool isNoopAddrSpaceCast(unsigned SrcAS,
                                    unsigned DstAS) const override;
 };
+
+class RISCVPassConfig : public TargetPassConfig {
+public:
+  RISCVPassConfig(RISCVTargetMachine &TM, PassManagerBase &PM)
+      : TargetPassConfig(TM, PM) {}
+
+  RISCVTargetMachine &getRISCVTargetMachine() const {
+    return getTM<RISCVTargetMachine>();
+  }
+
+  void addIRPasses() override;
+  bool addInstSelector() override;
+  bool addIRTranslator() override;
+  bool addLegalizeMachineIR() override;
+  bool addRegBankSelect() override;
+  bool addGlobalInstructionSelect() override;
+  void addPreEmitPass() override;
+  void addPreEmitPass2() override;
+  void addPreSched2() override;
+  void addMachineSSAOptimization() override;
+  void addPreRegAlloc() override;
+  void addPostRegAlloc() override;
+};
 } // namespace llvm
 
 #endif

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp

 #include "RISCVTargetTransformInfo.h"
 #include "TargetInfo/RISCVTargetInfo.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/CodeGen/GlobalISel/IRTranslator.h"
 #include "llvm/CodeGen/GlobalISel/InstructionSelect.h"
 #include "llvm/CodeGen/GlobalISel/Legalizer.h"
 #include "llvm/CodeGen/GlobalISel/RegBankSelect.h"
-#include "llvm/CodeGen/Passes.h"
 #include "llvm/CodeGen/TargetLoweringObjectFileImpl.h"
-#include "llvm/CodeGen/TargetPassConfig.h"
 #include "llvm/IR/LegacyPassManager.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Target/TargetOptions.h"
 using namespace llvm;
 
 static cl::opt<bool> EnableRedundantCopyElimination(

 // for all memory accesses, so it is reasonable to assume that an
 // implementation has no-op address space casts. If an implementation makes a
 // change to this, they can override it here.
 bool RISCVTargetMachine::isNoopAddrSpaceCast(unsigned SrcAS,
                                              unsigned DstAS) const {
   return true;
 }
 
-namespace {
-class RISCVPassConfig : public TargetPassConfig {
-public:
-  RISCVPassConfig(RISCVTargetMachine &TM, PassManagerBase &PM)
-      : TargetPassConfig(TM, PM) {}
-
-  RISCVTargetMachine &getRISCVTargetMachine() const {
-    return getTM<RISCVTargetMachine>();
-  }
-
-  void addIRPasses() override;
-  bool addInstSelector() override;
-  bool addIRTranslator() override;
-  bool addLegalizeMachineIR() override;
-  bool addRegBankSelect() override;
-  bool addGlobalInstructionSelect() override;
-  void addPreEmitPass() override;
-  void addPreEmitPass2() override;
-  void addPreSched2() override;
-  void addMachineSSAOptimization() override;
-  void addPreRegAlloc() override;
-  void addPostRegAlloc() override;
-};
-} // namespace
-
 TargetPassConfig *RISCVTargetMachine::createPassConfig(PassManagerBase &PM) {
   return new RISCVPassConfig(*this, PM);
 }
 
 void RISCVPassConfig::addIRPasses() {
   addPass(createAtomicExpandPass());
 
   addPass(createRISCVGatherScatterLoweringPass());