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

[Matt Davis via Phabricator via llvm-commits]

mattd added a comment.

Thanks a ton for your comments.  I'll simplify per our offline discussion, and what was grazed upon in your comments above.  Thanks.



================
Comment at: tools/llvm-mca/InstrBuilder.h:65
 
-  std::unique_ptr<Instruction> createInstruction(const llvm::MCInst &MCI);
+  Instruction *createInstruction(const llvm::MCInst &MCI);
 };
----------------
andreadb wrote:
> 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.
> 
My reason was to allow the caller decide how to manage the pointer.   The benefit was that we can treat the pointer as a shared, or unique, else where.  But given comments below, I'm convinced we can leave it as is.


================
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:
----------------
andreadb wrote:
> 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.
Those are all valid points.  My goal was to remove the Instructions list and any callbacks, such as eraseInstruction, to that list.   I was trying to remove the need for any Stage to have to callback-out to another stage or backend.  However, after discussing off-line, I am convinced to keep this Stage introduction simple.  While Instruction ownership did influence this patch, and I built Stage around that idea, I am convinced that we can just move the ownership to the FetchStage, at least for now.


================
Comment at: tools/llvm-mca/Stage.h:31
+  std::set<HWEventListener *> Listeners;
+  std::unique_ptr<Stage> NextStage;
+
----------------
andreadb wrote:
> 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.
The idea was that once a stage finishes with an instruction, it would then push that instruction to the next stage in the pipeline, very reactive.  However, for now, we can just manage the stages as a list in Backend.


https://reviews.llvm.org/D46741