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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Hi Clement,

Thanks for the feedback! Please see my answers below.

Cheers,
Andrea



================
Comment at: tools/llvm-mca/Backend.h:47
+/// over time.
+class Backend {
+  const MCSubtargetInfo &STI;
----------------
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?


================
Comment at: tools/llvm-mca/llvm-mca.cpp:315
+
+  std::unique_ptr<mca::Backend> B = llvm::make_unique<mca::Backend>(
+      *STI, *MCII, *MRI, std::move(S), Width, RegisterFileSize, MaxRetirePerCycle,
----------------
courbet wrote:
> I think that in the long run it should be possible for subtargets to specialize the creation of the backend. This would allow correctly modeling some finer implementation details. To that end we should add a createMCABackend() to llvm::Target. (in the future) .
I agree.


================
Comment at: utils/TableGen/SubtargetEmitter.cpp:611
 
 void SubtargetEmitter::EmitProcessorResources(const CodeGenProcModel &ProcModel,
                                               raw_ostream &OS) {
----------------
courbet wrote:
> I would split this to a separate diff and submit independently. These tables are so small that the added size is negligible.
Yes. That was the plan. This patch is essentially the union of two patches.
As you wrote, the change to this file is small enough to be committed independently (thanks to your previous patch on the processor resouce group descriptors :-)).

Since this is still an RFC, I plan to keep all the code into a single patch. Once/If people are happy with this design, then the change to Tablegen would be committed first.



https://reviews.llvm.org/D43951