[PATCH] D46741: [llvm-mca] Introduce a pipeline Stage class and FetchStage.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb requested changes to this revision.
andreadb added a comment.
This revision now requires changes to proceed.

Hi Matt,

Thanks for the patch! See my comments below.

-Andrea



================
Comment at: tools/llvm-mca/Backend.cpp:42
+  while (InitialStage->execute(IR)) {
+    assert(IR.isValid() && "Invalid InstRef produced by the initial stage.");
+    const InstrDesc &Desc = IR.getInstruction()->getDesc();
----------------
Remove this assert. By construction, you cannot have a null pointer from the SourceMgr.


================
Comment at: tools/llvm-mca/Backend.h:58
+  // decided by the backend implementation.
+  std::unique_ptr<Stage> InitialStage;
+
----------------
I would just call it FetchStage for now.


================
Comment at: tools/llvm-mca/Backend.h:86
+  void registerInitialStage(std::unique_ptr<Stage> IS) {
+    assert(IS && "Expected a non-nullptr initial stage instance.");
+    InitialStage = std::move(IS);
----------------
I would remove this for now.
It is clear that this is never going to be null.


================
Comment at: tools/llvm-mca/FetchStage.cpp:1
+//===---------------------- FetchStage.h ------------------------*- C++ -*-===//
+//
----------------
It should be `FetchStage.cpp`.


================
Comment at: tools/llvm-mca/InstrBuilder.cpp:430-433
+Instruction*
 InstrBuilder::createInstruction(const MCInst &MCI) {
   const InstrDesc &D = getOrCreateInstrDesc(MCI);
+  Instruction *NewIS = new Instruction(D);
----------------
It should return a unique_ptr.


================
Comment at: tools/llvm-mca/InstrBuilder.h:65
 
-  std::unique_ptr<Instruction> createInstruction(const llvm::MCInst &MCI);
+  Instruction *createInstruction(const llvm::MCInst &MCI);
 };
----------------
I don't like the changes to InstrBuilder.h and InstrBuilder.cpp.

I don't think that there is a good reason why createInstruction should return a normal pointer instead of a unique_ptr.



================
Comment at: tools/llvm-mca/Instruction.h:39
 /// this index as a key throughout MCA.
-class InstRef : public std::pair<unsigned, Instruction *> {
+class InstRef : public std::pair<unsigned, std::shared_ptr<Instruction>> {
 public:
----------------
I've been thinking about the instruction ownership problem.

My opinion is that there is no real advantage in passing the ownership of an instruction from stage to stage.
It complicates the logic for very little (if no) gain. It forces the instruction to be manipulated as a shared_ptr in the Scheduler.

I'd like to keep the code simple. I prefer a simpler (and more intuitive) approach where instructions are created and destroyed in the same place.

Any particular reason why the FetchStage cannot be the owner of all Instructions? At the end of each cycle, it looks for instructions that have reached the retired stage, and remove them. That approach would be much cleaner, and - more importantly - it would not require using shared_ptr everywhere.


================
Comment at: tools/llvm-mca/Stage.h:28
+
+class Stage {
+  std::string Name;
----------------
Create a Stage.cpp, and pin the vtable there.
Move the definition of `registerNextStage` and `addListener` to Stage.cpp.


================
Comment at: tools/llvm-mca/Stage.h:29
+class Stage {
+  std::string Name;
+  std::set<HWEventListener *> Listeners;
----------------
Please remove this field.


================
Comment at: tools/llvm-mca/Stage.h:31
+  std::set<HWEventListener *> Listeners;
+  std::unique_ptr<Stage> NextStage;
+
----------------
A stage should not "own" the next stage. I think the Backend (i.e. our pipeline) should be the owner of all the stages. Please remove this unique_ptr.


================
Comment at: tools/llvm-mca/Stage.h:36-37
+  virtual ~Stage() = default;
+  Stage(const Stage &Other) = delete;
+  Stage &operator=(const Stage &Other) = delete;
+
----------------
Make these two to private.


https://reviews.llvm.org/D46741