All target knowledge should be in the TargetMachine base
class. LLVMTargetMachine extends with pass machinery, which most code
should not have access to.
Details
Diff Detail
Event Timeline
llvm/include/llvm/Target/TargetMachine.h | ||
---|---|---|
137 | It doesn't really change anything, but this one doesn't fit the theme of the what LLVMTargetMachine adds. This is core target information and it doesn't make much since to have it in LLVMTargetMachine, which only adds pass machinery controls. |
All target knowledge should be in the TargetMachine base class. LLVMTargetMachine extends with pass machinery, which most code should not have access to.
My understanding was that TargetMachine is the public/outside interface (for frontends etc.). And that LLVMTargetMachine is a concrete subclass for all targets implemented using lib/CodeGen. In theory you can implement a complete target from scratch without using lib/CodeGen and then you would implement TargetMachine without using LLVMTargetMachine.
So to me this change looks confusing/unnecessary, since any code within lib/CodeGen is totally free to use LLVMTargetMachine in my interpretation...
Requesting changes for clarification
llvm/include/llvm/Target/TargetMachine.h | ||
---|---|---|
137 | This seems like an implementation detail of lib/CodeGen so I think it should stay withint LLVMTargetMachine. |
This sounds backwards to me. LLVMTargetMachine extends with pieces frontends need to use the target (e.g. createMCStreamer). TargetMachine is the base target info class codegen does use. What you're saying would make more sense if the relationship was inverted?
The current situation is that I'm not sure any code makes a practical distinction here. For example to construct a MachineModuleInfo, the existing users downcast from TargetMachine to LLVMTargetMachine
This sounds backwards to me. LLVMTargetMachine extends with pieces frontends need to use the target (e.g. createMCStreamer). TargetMachine is the base target info class codegen does use. What you're saying would make more sense if the relationship was inverted?
I remember spending quite a while wondering about this myself; I couldn't find anyone who could actually explain the TargetMachine / LLVMTargetMachine split to me.
So my reading is mostly based on the fact that the TargetRegistry API that frontends need to use to construct a target and especially the createTargetMachine() call there returns a TargetMachine and not an LLVMTargetMachine. So it seemed to me like LLVMTargetMachine was intended as an implementation detail for lib/CodeGen. You can also see clang code only dealing with llvm::TargetMachine but not LLVMTargetMachine.
I agree that this is all convoluted and not consistent and becomes in no way clear given the class names. I'm currently not aware of any code implementing a TargetMachine but not LLVMTargetMachine. So maybe we should just kill this whole discussion and merge the two classes into just TargetMachine?
It seems I actually discussed this before in https://lists.llvm.org/pipermail/llvm-dev/2017-October/thread.html#117907 and we had no opposition for moving the two classes. Seems I just never got around to actually do it...
It seems I did try to merge the two in https://reviews.llvm.org/D38489 / https://github.com/llvm/llvm-project/commit/3a9c114b2418472b16276e95d3f657735926dddb but ended up reverting in bb8507e63c42212c3753c0df069ec16a52652247 (which admittedly I'm not sure I understand the problem anymore now...)
The discussion in https://reviews.llvm.org/D38489 seems to indicate that the problem is that some users/binaries link libTarget but not libCodeGen and they expect TargetMachine to work but not LLVMTargetMachine.
So based on the uses with libCodeGen=TargetMachine, libTarget=LLVMTargetMachine,
usesPhysRegsForValues and unqualifiedInlineAsmVariant ared used in lib/CodeGen, so belong in TargetMachine which is what this patch does. It also tightens some of the references in libCodeGen to TargetMachine, so this patch is in line with that reasoning?
No?
- TargetMachine is defined in libTarget (lib/Target/TargetMachine.cpp)
- LLVMTargetMachine is defined in libCodegen (lib/CodeGen/LLVMTargetMachine.cpp) and libCodeGen links to libTarget (but not the other way round).
Note that libTarget is not the same as actual targets like lib/Target/X86... The actual targets do link against libCodeGen... (it's all so confusing)
This (and the below) with a reference are a little disturbing. (I know you didn't change it, but...)