[PATCH] D43951: [RFC] llvm-mca: a static performance analysis tool.

Clement Courbet via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Mar 2 06:27:37 PST 2018


courbet added inline comments.


================
Comment at: tools/llvm-mca/Backend.h:47
+/// over time.
+class Backend {
+  const MCSubtargetInfo &STI;
----------------
andreadb wrote:
> courbet wrote:
> > My main reservation is around the fact that there is a lot of coupling between the driver (RunCycle()/NotifyCycleBegin()/NotifyCycleEnd()) and the  pipeline itself. I think this should be split into a driver class ad a configurable pipeline.
> > With the current design the backend has to know about all the possible pipelines and expose all the quantities that listeners might be interested in (GetNumXXXStalls(), NotifyInstructionXXX()). This does not scale well to more pipeline components (in particular to simulate the frontend). I think it would be better to manipulate IB/HWS/DU though a common interface and give this interface a way to notify the listeners.
> > What do you think ?
> I agree with you that this is what should be done in the medium/long term. I think it is a must if we want to let targets compose the sequence of stages.
> 
> When I originally designed this tool, I adopted a similar approach to the one you described, where every component was able to notify events to listeners through a common interface.
> However, I ended up adopting this simpler but "less modular" design mainly for simplicity; the number of component/stages was not "big enough" to justify the presence of multiple event producers. Essentially, having the Backend as the single publisher of events was much simpler.
> 
> As I mentioned in the RFC, this version of the tool doesn currently model the hardware frontend. The long term goal is to teach the tool how to analyze frontend performance too. So, I would be very happy if you guys at Google would help contributing that part!
> 
> In the meantime, would it be possible to have this changed in a follow-up patch in preparation for the "future" development?
> So, I would be very happy if you guys at Google would help contributing that part!
> In the meantime, would it be possible to have this changed in a follow-up patch in preparation for the "future" development?

SGTM.


https://reviews.llvm.org/D43951





More information about the llvm-commits mailing list