Many target MCTargetDesc libraries still include headers from the target CodeGen library #64166

rnk · 2023-07-27T18:22:38Z

Every LLVM target is composed of several libraries. The primary library, the one called LLVM${TGT}CodeGen, is responsible for translating IR to MachineInstrs, and ultimately "printing" (or producing) MCInstrs, which is our representation of assembly. Below that, we have the MCTargetDesc library, which is intended to contain data about the ISA. This should not depend on CodeGen. The intention is that we can link llvm-mc for all targets without linking in either the target-independent library (LLVMCodeGen) or the target-specific codegen library (LLVMX86CodeGen).

This is how things are linked together in CMake. The problem is that our headers are not well organized, and we still reference headers from llvm/CodeGen/ from files in MCTargetDesc. This problem is detected by Bazel, which sandboxes each action and stages the necessary inputs to every action. If you apply this patch to remove the dep on CodeGen from all the MCTargetDesc libraries, you can observe compilation failures.

I have a stack of patches in a github branch to fix this issue. I'm filing this issue to describe the problem well once so I can link to it from the commit messages.

The error message from Bazel typically looks like a file-not-found error on an include, something like this:

In file included from external/llvm-project/llvm/lib/Target/AMDGPU/MCA/AMDGPUCustomBehaviour.cpp:16:                                                                                                                                                                                      
In file included from bazel-out/k8-fastbuild/bin/external/llvm-project/llvm/_virtual_includes/AMDGPUUtilsAndDesc/SIInstrInfo.h:17:                                                                                                                                                        
bazel-out/k8-fastbuild/bin/external/llvm-project/llvm/_virtual_includes/AMDGPUUtilsAndDesc/AMDGPUMIRFormatter.h:19:10: fatal error: 'llvm/CodeGen/MIRFormatter.h' file not found                                                                                                          
#include "llvm/CodeGen/MIRFormatter.h"                                                                                                                                                                                                                                                    
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                                                                                       
1 error generated.

Aside from Bazel, we should fix these issues because it makes our code fragile. We reference these CodeGen headers but do not link against the corresponding cpp file, so the link will fail if someone decides to move an inline function out of line or to call a non-inline method.

The text was updated successfully, but these errors were encountered:

These are small include-only changes in the X86, Mips, and SystemZ backend that seem sufficiently small to commit separately without review. See issue #64166 for more information about layering.

These are small include-only changes in the AArch64 and ARM backends that seem sufficiently small to commit separately without review. See issue #64166 for more information about layering.

This required two substantial changes: 1. Moving a `getRegBitWidth(TargetRegisterClass)` overload out of Utils and into CodeGen 2. Passing the string function name to AMDGPUPALMetadata instead of the MachineFunction Other changes are minor or updates to accommodate the first two. See issue #64166 for more information on the layering issue. Differential Revision: https://reviews.llvm.org/D156486

These are small include-only changes in the X86, Mips, and SystemZ backend that seem sufficiently small to commit separately without review. See issue llvm#64166 for more information about layering.

These are small include-only changes in the AArch64 and ARM backends that seem sufficiently small to commit separately without review. See issue llvm#64166 for more information about layering.

This required two substantial changes: 1. Moving a `getRegBitWidth(TargetRegisterClass)` overload out of Utils and into CodeGen 2. Passing the string function name to AMDGPUPALMetadata instead of the MachineFunction Other changes are minor or updates to accommodate the first two. See issue llvm#64166 for more information on the layering issue. Differential Revision: https://reviews.llvm.org/D156486

Move WebAssemblyUtilities from Utils to the CodeGen library. It primarily deals in MIR layer types, so it really lives in the CodeGen library. Move a variety of other things around to try create better separation. See issue #64166 for more info on layering. Move llvm/include/CodeGen/WasmAddressSpaces.h back to llvm/lib/Target/WebAssembly/Utils. Differential Revision: https://reviews.llvm.org/D156472

MaskRay · 2023-08-18T22:33:17Z

Thank you for working on this!

My f406db9 added -layering_check to name = target["name"] + "UtilsAndDesc" so that I can enable layering_check for llvm/. There are other related issues here...

…eps, NFC See issue #64166 for more information about layering.

See issue #64166 for more information about the layering issue. The PPCMCTargetDesc library was including CodeGen headers such as PPCInstrInfo.h and calling inline functions in them. This doesn't work in the Bazel build, and is error-prone. If the inline function moves to a cpp file, it will result in linker errors. To address the issue, I moved several inline functions to PPCMCTargetDesc.cpp, and declared them in the PPC namespace in PPCMCTargetDesc.h, which seemed like the most straightforward fix. Differential Revision: https://reviews.llvm.org/D156488

See issue #64166 for more information about the layering issue.

Move WebAssemblyUtilities from Utils to the CodeGen library. It primarily deals in MIR layer types, so it really lives in the CodeGen library. Move a variety of other things around to try create better separation. See issue llvm#64166 for more info on layering. Move llvm/include/CodeGen/WasmAddressSpaces.h back to llvm/lib/Target/WebAssembly/Utils. Differential Revision: https://reviews.llvm.org/D156472