Page MenuHomePhabricator

[MCA] Moving the target specific CustomBehaviour impl. from /tools/llvm-mca/ to /lib/Target/.
ClosedPublic

Authored by holland11 on Jul 25 2021, 2:43 PM.

Details

Summary

This patch is related to https://reviews.llvm.org/D104149 and https://reviews.llvm.org/D104730 . The first patch introduced the CustomBehaviour class to llvm-mca. To start, target specific CBs were meant to be placed within the llvm-mca directory. Since the class is only used by llvm-mca, this made sense and was the cleanest and most intuitive place to put them.

However, in the second patch, I attempted to implement some custom modelling for AMDGPU. This modelling required the use of some backend functions and symbols. This works fine for static builds and for shared builds that are not on linux. However, when building with shared libs on linux, there were build errors because the backend symbols were not accessible by AMDGPUCB. For more details on this, you can refer to the most recent comments within the second diff linked above.

To address these errors, I restructured the target specific CB implementations to be placed within the /lib/Target/<Target-Name>/MCA directory. (This MCA directory is new.) Since the implementations are now a part of the target's backend, they need to be initialized and registered similar to how many other target specific classes work (such as AsmParser). To accomplish this, I had to add some code to several cmake files throughout llvm. This is my first time working with general llvm config stuff so if I made any mistakes, hopefully someone can spot them and help me correct them.

This patch keeps the AMDGPUCB implementation empty. If/when this patch gets approved and pushed, I will submit a diff for the AMDGPUCB implementation (which is the same implementation seen in the second diff linked above).

I have tested several builds and run ninja check all on them on both macOS and linux. I have tested them with the future AMDGPUCB implementation as well to make sure that this solution does in fact fix the original error regarding unresolved symbols when building with shared libs on linux.

If anybody sees any issues or has any suggestions for ways that this can be done better, I'd be happy to tweak or modify this patch.

Thanks for your time!

Diff Detail

Event Timeline

holland11 created this revision.Jul 25 2021, 2:43 PM
holland11 requested review of this revision.Jul 25 2021, 2:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2021, 2:43 PM
holland11 edited the summary of this revision. (Show Details)Jul 25 2021, 2:46 PM
holland11 updated this revision to Diff 361608.Jul 26 2021, 3:20 AM
andreadb accepted this revision.Jul 26 2021, 3:31 AM

Looks good to me.

This revision is now accepted and ready to land.Jul 26 2021, 3:31 AM
foad added inline comments.Jul 26 2021, 7:26 AM
llvm/lib/Target/AMDGPU/CMakeLists.txt
159

I don't speak cmake. What does this line do? Does it mean that the target MCA class will be pulled into the AMDGPUCodeGen library? That seems unnecessary, since it's not needed for codegen.

andreadb added inline comments.
llvm/lib/Target/AMDGPU/CMakeLists.txt
159

That's correct.

In https://reviews.llvm.org/D104730, Patrick has suggested three options for solving this issue. This was his comment:

@tstellar @andreadb I've been thinking about the problem for a bit now and have been brainstorming different ideas and digging through the code. I just made a post on the llvm discourse to hopefully get some feedback on the issue (and some potential solutions that I've come up with). If you guys are curious or would be willing to take a look, here's the post https://llvm.discourse.group/t/need-help-deciding-how-to-access-backend-functions-from-within-a-tool/3915 .

On that discourse link, both me and Tom have suggested to implement option 2. (i.e. this patch). We believe that it is the simplest solution, and we don't expect the CustomBehaviour code to ever be big enough in general.

I agree that this change is not strictly needed for codegen now. However, that might change in future.

During the development of MCA,it was requested to make MCA accessible from Codegen. That is because a few people (including @courbet 's team) expressed the intention of using MCA to define their own custom machine scheduler passes.

Unfortunately, we never really got to that point (mainly due to the lack of devs time). Nowadays, MCA is also built as a library. However, it is missing some lowering functionalities to translate from MachineInstr to mca::Instruction. However, that could change in future; if so, then this patch would make things much easier.

That being said, the main reason why we suggested to use this approach is mainly because we assumed that the impact on Codegen of CustomBehaviour objects would always be relatively small. Maybe Patrick could give a bit more details on this (since he presumably has also some numbers to show).

foad added inline comments.Jul 26 2021, 9:00 AM
llvm/lib/Target/AMDGPU/CMakeLists.txt
159

Thanks for explaining. I don't object to this, just wanted to make sure I understood how it worked.

holland11 updated this revision to Diff 362152.Jul 27 2021, 1:11 PM

I have just removed the AMDGPUTargetMCA line from the AMDGPUCodeGen cmake file. I was originally just following the patterns that I saw from the other folders and classes in that directory and then afterwards I went through and tried to remove things that didn't seem necessary. I guess I missed that one.

Tested it on both macOS and linux and it seems to still work properly. CodeGen definitely doesn't depend on TargetMCA so it was not necessary to have that line.

Will let the build test do its thing and if it looks good, I'll push this patch and watch the build bots closely in case I need to revert.

Have you looked at how per-target stuff of llvm-mca is handled?
Does that also need similar changes? If not, perhaps this can be changed to more closely mimic that instead?

holland11 added a comment.EditedJul 27 2021, 1:37 PM

Have you looked at how per-target stuff of llvm-mca is handled?
Does that also need similar changes? If not, perhaps this can be changed to more closely mimic that instead?

As far as MCA specific stuff goes, this is the only 'per-target' code that I'm aware of. MCA does use target specific classes (that aren't exclusive to mca) such as AsmParser, MCTargetStreamer, etc. and those are what I based this implementation off of.

I have just removed the AMDGPUTargetMCA line from the AMDGPUCodeGen cmake file. I was originally just following the patterns that I saw from the other folders and classes in that directory and then afterwards I went through and tried to remove things that didn't seem necessary. I guess I missed that one.

Tested it on both macOS and linux and it seems to still work properly. CodeGen definitely doesn't depend on TargetMCA so it was not necessary to have that line.

Will let the build test do its thing and if it looks good, I'll push this patch and watch the build bots closely in case I need to revert.

Sounds good. Thanks!

This revision was landed with ongoing or failed builds.Jul 28 2021, 11:23 AM
This revision was automatically updated to reflect the committed changes.