This is an archive of the discontinued LLVM Phabricator instance.

[llvm-mca] Turn InstructionTables into a Stage.
ClosedPublic

Authored by mattd on Jul 13 2018, 4:17 PM.

Details

Summary

This patch converts the InstructionTables class into a subclass of mca::Stage. This change allows us to use the Stage's inherited Listeners for event notifications. This also allows us to create a simple pipeline for viewing the InstructionTables report.

I have been working on a follow on patch that should cleanup addView in InstructionTables. Right now, addView adds the view to both the Listener list and Views list. The follow-on patch addresses the fact that we don't really need two lists in this case. That change is not specific to just InstructionTables, so it will be a separate patch.

Diff Detail

Repository
rL LLVM

Event Timeline

mattd created this revision.Jul 13 2018, 4:17 PM
mattd edited the summary of this revision. (Show Details)Jul 13 2018, 4:18 PM
mattd added inline comments.
tools/llvm-mca/InstructionTables.h
40 ↗(On Diff #155529)

I'm working on a follow on patch to clean this addListener call up.

tools/llvm-mca/llvm-mca.cpp
507 ↗(On Diff #155529)

A follow on patch will clean this printReport up.

mattd edited the summary of this revision. (Show Details)Jul 13 2018, 10:38 PM
mattd added inline comments.
tools/llvm-mca/llvm-mca.cpp
507 ↗(On Diff #155529)

It doesn't look like the follow-on patch will clean this line up.

andreadb requested changes to this revision.Jul 14 2018, 5:23 AM

Hi Matt,

Thanks for working on this.
There are a couple of changes that I don't quite understand. Unless I am missing something, I think the patch could have been a lot simpler. See my comments below.

Thanks,
-Andrea

tools/llvm-mca/InstructionTables.cpp
35–58 ↗(On Diff #155529)

We don't tend to add const to local variables with primitive types. Although, technically it is not wrong, it needlessly increases the level of verbosity for no good reason (no value in return).
In several parts of LLVM, const is mainly associated with reference/pointer types, mainly to avoid that non-const methods are called on them, and to emphasize that data is not changed through those pointers.

In this particular case, adding const doesn't make things better imho. I also don't want that we end up doing similar changes to other files of this project. Since this change is not really needed to fix the issue with the patch, I suggest to remove it if you don't mind.

tools/llvm-mca/InstructionTables.h
32 ↗(On Diff #155529)

This is not needed.

40 ↗(On Diff #155529)

Hang on.
You shouldn't have this method at all! Just use a PipelinePrinter to generate the report.

tools/llvm-mca/llvm-mca.cpp
493–507 ↗(On Diff #155529)

Unless I am missing something, I think you should simply use a PipelinePrinter. I don't think that any follow-on patch is actually needed here. If you use a PipelinePrinter, then it becomes the owner of all the views.

494–499 ↗(On Diff #155529)

Why this? You should be able to use a PipelinePrinter, and add View objects directly to the printer.
The printer would then subscribe views to each stage of the pipeline.

This revision now requires changes to proceed.Jul 14 2018, 5:23 AM
mattd updated this revision to Diff 155566.Jul 14 2018, 12:10 PM
mattd marked 4 inline comments as done.
  • Removed some constified locals (my style/habit)
  • Yanked out the original Printer functionality from InstructionTables, and use the PipelinePrinter instead.
mattd marked an inline comment as done.Jul 14 2018, 12:11 PM
mattd added inline comments.
tools/llvm-mca/InstructionTables.cpp
35–58 ↗(On Diff #155529)

Fair point. I tend to do this as part of my personal style, indicating that a value will not change, implying that there's no need to hunt down any writes to the variable later on. But your point is clear, I'll undo it.

tools/llvm-mca/InstructionTables.h
40 ↗(On Diff #155529)

I didn't consider a PipelinePrinter, I kept the InstructionTables close to its original structure with its original Printer-like representation (addView/printReport/Views). PipelinePrinter will remove the need for those bits, sorry for that.

Note that InstructionTables is now a Stage, with listeners. We will still need to associate the events generated by the stage to the views. This means we will need to make a call to addListener to IT. If we don't do this, then the resource pressure view will not get any data.

mattd updated this revision to Diff 155570.Jul 14 2018, 1:30 PM
  • Move the adding of stages to the pipeline earlier, so that they automatically register view-listeners when views are added to the printer.
mattd added inline comments.Jul 14 2018, 1:32 PM
tools/llvm-mca/InstructionTables.h
40 ↗(On Diff #155529)

The code now relies on the addView in Pipeline to register the views to the pipeline's stages. PipelinePrinter is definitely the way to go here, thanks for the suggestion.

andreadb accepted this revision.Jul 14 2018, 3:17 PM

Nice!

LGTM.

Thanks for working on this!

This revision is now accepted and ready to land.Jul 14 2018, 3:17 PM
This revision was automatically updated to reflect the committed changes.