This is an archive of the discontinued LLVM Phabricator instance.

[llvm-mca] Introduce the llvm-mca library and organize the directory accordingly. NFC.
ClosedPublic

Authored by mattd on Aug 17 2018, 3:38 PM.

Details

Summary

This patch introduces llvm-mca as a library. The driver (llvm-mca.cpp), views, and stats, are not part of the library.
Those are separate components that are not required for the functioning of llvm-mca.

The directory has been organized as follows:
All library source files now reside in:

  • lib/HardwareUnits/ - All subclasses of HardwareUnit (these represent the simulated hardware components of a backend). (LSUnit does not inherit from HardwareUnit, but Scheduler does which uses LSUnit).
  • lib/Stages/ - All subclasses of the pipeline stages.
  • lib/ - This is the root of the library and contains library code that does not fit into the Stages or HardwareUnit subdirs.

All library header files now reside in the include directory and mimic the same layout as the lib directory mentioned above.

In the (near) future we would like to move the library (include and lib) contents from tools and into the core of llvm somewhere.
That change would allow various analysis and optimization passes to make use of MCA functionality for things like cost modeling.

I left all of the non-library code just where it has always been, in the root of the llvm-mca directory.
The include directives for the non-library source file have been updated to refer to the llvm-mca library headers.
I updated the llvm-mca/CMakeLists.txt file to include the library headers, but I made the non-library code
explicitly reference the library's 'include' directory. Once we eventually (hopefully) migrate the MCA library
components into llvm the include directives used by the non-library source files will be updated to point to the
proper location in llvm.

Diff Detail

Event Timeline

mattd created this revision.Aug 17 2018, 3:38 PM
mattd added inline comments.Aug 17 2018, 3:41 PM
tools/llvm-mca/PipelinePrinter.h
21

As I mentioned in the description. Once we move llvm-mca into a core llvm library, these 'include/' subdirectories will be updated to reference that location. For now, I left the include subdirectory explicit, even though the CMake file already includes that subdirectory.

mattd updated this revision to Diff 161379.Aug 18 2018, 12:18 PM
  • Tossed in an LLVMBuild.txt for the library.
  • Added library dependencies to the cmake.

My opinion is that CodeRegion.h and CodeRegion.cpp should not be part of the library. Those files implement a class which is meant to be used by the llvm-mca driver only to mark regions of code. A different utility class/approach should be used when llvm-mca is used as a library.

I would like to place the Views and Stats into separate (non-library) directories, just to keep things better organized, but that would be a separate patch.

Any reasons why it cannot be done as part of this patch?

mattd added a comment.Aug 20 2018, 9:44 AM

My opinion is that CodeRegion.h and CodeRegion.cpp should not be part of the library. Those files implement a class which is meant to be used by the llvm-mca driver only to mark regions of code. A different utility class/approach should be used when llvm-mca is used as a library.

I am fine with that, I'll move those utility pieces outside of the library.

I would like to place the Views and Stats into separate (non-library) directories, just to keep things better organized, but that would be a separate patch.

Any reasons why it cannot be done as part of this patch?

I was just trying to isolate this patch to the addition of a library, but I am happy to move those files in this patch too.

mattd updated this revision to Diff 162210.Aug 23 2018, 9:59 AM
  • Updated per comments from the last spin:
    • Moved CodeRegion out of the library.
    • Added Views and Stats directories to cleanup the layout (Views and Stats are not part of the library).
mattd updated this revision to Diff 162421.Aug 24 2018, 10:59 AM
  • Move the Resource classes into their own file.
mattd updated this revision to Diff 162511.Aug 24 2018, 5:07 PM

Rebased this diff after reorganizing the llvm-mca directory some.

mattd edited the summary of this revision. (Show Details)Aug 24 2018, 5:14 PM
andreadb accepted this revision.Aug 27 2018, 3:56 AM

Hi Matt.

Thanks for this refactoring.
The change looks good to me.

Please update https://bugs.llvm.org/show_bug.cgi?id=37696 once you have committed this change. Thanks!

(also: unrelated to this patch. When you have time, could you please update https://bugs.llvm.org/show_bug.cgi?id=37835 ?).

-Andrea

This revision is now accepted and ready to land.Aug 27 2018, 3:56 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2019, 12:23 PM