I have mixed feeling about this. While this does separate out target-independent pieces into CodeGen, it also introduces some confusion where the default implementation is in CodeGen while target overridden version are in Target. I also hate to see all these Target* classes being moved to CodeGen. I thought our solution to this issue is classes such as TargetInstrInfoImpl. What's wrong with it? Evan On Dec 10, 2011, at 7:25 PM, Nick Lewycky wrote:> Nick Lewycky wrote: >> We've had a circular dependency in LLVM for a while, and while we've >> been fortunate that we could ignore it by implementing functions in >> header files, a recent innocent change caused a cyclic dependency >> between Target and CodeGen just because of inlining that happens in >> GCC. I'm proposing to fix this by moving code from Target to CodeGen >> >> If I understand correctly, lib/CodeGen is target-independent code >> generator parts and lib/Target provides a target-independent interface >> that the code in CodeGen uses, while lib/Target/Foo provides >> target-specific data for, and implementations of, those target >> independent pieces. The targets themselves have access to both CodeGen >> and Target, and those Target-independent interfaces are defined in >> terms of types in CodeGen. >> >> This means we have two target-independent layers, one in lib/Target >> (excluding subdirs) and lib/CodeGen. I think the right thing to do is >> move the pieces in lib/Target which mention CodeGen types (for example >> TargetInstrInfo) into CodeGen. Please let me know whether this is the >> right thing to do, and meanwhile I'm going to try to prepare a patch >> that does this. > > I've gone ahead and done this. With the attached patch applied, nothing in lib/Target/* or include/llvm/Target/* include anything from include/llvm/CodeGen. > > The .cpp files that moved are TargetELFWriterInfo, TargetFrameLowering, TargetLoweringObjectFile, TargetMachine, TargetOptions, TargetRegisterInfo and TargetSubtargetInfo. The .h files that moved are the matching headers for the .cpp files plus TargetInstrInfo, TargetSelectionDAGInfo and TargetLowering. > > Please review! More generally, I suggest reviewing the idea of moving everything rather than looking at the patch itself. The patch is lots of changing Target/X to CodeGen/X, nothing exciting. > > Nick > <target-no-codegen-llvm-1.patch><target-no-codegen-clang-1.patch>
On 12 December 2011 12:22, Evan Cheng <evan.cheng at apple.com> wrote:> I have mixed feeling about this. While this does separate out target-independent pieces into CodeGen, it also introduces some confusion where the default implementation is in CodeGen while target overridden version are in Target. I also hate to see all these Target* classes being moved to CodeGen. > > I thought our solution to this issue is classes such as TargetInstrInfoImpl. What's wrong with it?I wasn't familiar with TargetFooImpl when I sent out the initial mail. I don't like it because it splits the implementation of the class across two entirely separate libraries (not merely source files), it's easy to accidentally regress the dependency, it doesn't make sense to have two target-independent layers especially given that they clearly want to depend on each other, and the end state with your TargetFooImpl.cpp approach is that we'll keep adding more and more Target...Impl files into CodeGen (as Target keeps using CodeGen) which is what you said you hate to see in my patch. I figured if that's where we're headed, I would just move everything at once, but we can also do it in pieces. The other thing I wanted is to make sure that the $DIR in include/$DIR/File.h matches lib/$DIR/File.cpp for its implementation. Since you're already splitting an implementation of a single class across two libraries, I would guess that you also don't care whether the header files are in the matching directories. The new attached patch moves all the code in .cpp files in Target that depends on CodeGen into *Impl.cpp files in CodeGen. I did not address header files in this patch. Nick> > Evan > > On Dec 10, 2011, at 7:25 PM, Nick Lewycky wrote: > >> Nick Lewycky wrote: >>> We've had a circular dependency in LLVM for a while, and while we've >>> been fortunate that we could ignore it by implementing functions in >>> header files, a recent innocent change caused a cyclic dependency >>> between Target and CodeGen just because of inlining that happens in >>> GCC. I'm proposing to fix this by moving code from Target to CodeGen >>> >>> If I understand correctly, lib/CodeGen is target-independent code >>> generator parts and lib/Target provides a target-independent interface >>> that the code in CodeGen uses, while lib/Target/Foo provides >>> target-specific data for, and implementations of, those target >>> independent pieces. The targets themselves have access to both CodeGen >>> and Target, and those Target-independent interfaces are defined in >>> terms of types in CodeGen. >>> >>> This means we have two target-independent layers, one in lib/Target >>> (excluding subdirs) and lib/CodeGen. I think the right thing to do is >>> move the pieces in lib/Target which mention CodeGen types (for example >>> TargetInstrInfo) into CodeGen. Please let me know whether this is the >>> right thing to do, and meanwhile I'm going to try to prepare a patch >>> that does this. >> >> I've gone ahead and done this. With the attached patch applied, nothing in lib/Target/* or include/llvm/Target/* include anything from include/llvm/CodeGen. >> >> The .cpp files that moved are TargetELFWriterInfo, TargetFrameLowering, TargetLoweringObjectFile, TargetMachine, TargetOptions, TargetRegisterInfo and TargetSubtargetInfo. The .h files that moved are the matching headers for the .cpp files plus TargetInstrInfo, TargetSelectionDAGInfo and TargetLowering. >> >> Please review! More generally, I suggest reviewing the idea of moving everything rather than looking at the patch itself. The patch is lots of changing Target/X to CodeGen/X, nothing exciting. >> >> Nick >> <target-no-codegen-llvm-1.patch><target-no-codegen-clang-1.patch> > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev-------------- next part -------------- A non-text attachment was scrubbed... Name: target-no-codegen-llvm-2.patch Type: text/x-patch Size: 10068 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20111212/31f1da14/attachment.bin>
I prefer this approach. I don't think there is a perfect solution but it's a step in the right direction. It's going to take some effort to really separate out the parts of libLLVMTarget that are truly independent from those that are only needed for code generation. Evan On Dec 12, 2011, at 3:18 PM, Nick Lewycky wrote:> On 12 December 2011 12:22, Evan Cheng <evan.cheng at apple.com> wrote: >> I have mixed feeling about this. While this does separate out target-independent pieces into CodeGen, it also introduces some confusion where the default implementation is in CodeGen while target overridden version are in Target. I also hate to see all these Target* classes being moved to CodeGen. >> >> I thought our solution to this issue is classes such as TargetInstrInfoImpl. What's wrong with it? > > I wasn't familiar with TargetFooImpl when I sent out the initial mail. > I don't like it because it splits the implementation of the class > across two entirely separate libraries (not merely source files), it's > easy to accidentally regress the dependency, it doesn't make sense to > have two target-independent layers especially given that they clearly > want to depend on each other, and the end state with your > TargetFooImpl.cpp approach is that we'll keep adding more and more > Target...Impl files into CodeGen (as Target keeps using CodeGen) which > is what you said you hate to see in my patch. I figured if that's > where we're headed, I would just move everything at once, but we can > also do it in pieces. > > The other thing I wanted is to make sure that the $DIR in > include/$DIR/File.h matches lib/$DIR/File.cpp for its implementation. > Since you're already splitting an implementation of a single class > across two libraries, I would guess that you also don't care whether > the header files are in the matching directories. > > The new attached patch moves all the code in .cpp files in Target that > depends on CodeGen into *Impl.cpp files in CodeGen. I did not address > header files in this patch. > > Nick > >> >> Evan >> >> On Dec 10, 2011, at 7:25 PM, Nick Lewycky wrote: >> >>> Nick Lewycky wrote: >>>> We've had a circular dependency in LLVM for a while, and while we've >>>> been fortunate that we could ignore it by implementing functions in >>>> header files, a recent innocent change caused a cyclic dependency >>>> between Target and CodeGen just because of inlining that happens in >>>> GCC. I'm proposing to fix this by moving code from Target to CodeGen >>>> >>>> If I understand correctly, lib/CodeGen is target-independent code >>>> generator parts and lib/Target provides a target-independent interface >>>> that the code in CodeGen uses, while lib/Target/Foo provides >>>> target-specific data for, and implementations of, those target >>>> independent pieces. The targets themselves have access to both CodeGen >>>> and Target, and those Target-independent interfaces are defined in >>>> terms of types in CodeGen. >>>> >>>> This means we have two target-independent layers, one in lib/Target >>>> (excluding subdirs) and lib/CodeGen. I think the right thing to do is >>>> move the pieces in lib/Target which mention CodeGen types (for example >>>> TargetInstrInfo) into CodeGen. Please let me know whether this is the >>>> right thing to do, and meanwhile I'm going to try to prepare a patch >>>> that does this. >>> >>> I've gone ahead and done this. With the attached patch applied, nothing in lib/Target/* or include/llvm/Target/* include anything from include/llvm/CodeGen. >>> >>> The .cpp files that moved are TargetELFWriterInfo, TargetFrameLowering, TargetLoweringObjectFile, TargetMachine, TargetOptions, TargetRegisterInfo and TargetSubtargetInfo. The .h files that moved are the matching headers for the .cpp files plus TargetInstrInfo, TargetSelectionDAGInfo and TargetLowering. >>> >>> Please review! More generally, I suggest reviewing the idea of moving everything rather than looking at the patch itself. The patch is lots of changing Target/X to CodeGen/X, nothing exciting. >>> >>> Nick >>> <target-no-codegen-llvm-1.patch><target-no-codegen-clang-1.patch> >> >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > <target-no-codegen-llvm-2.patch>
Maybe Matching Threads
- [LLVMdev] moving from lib/Target and lib/CodeGen
- [LLVMdev] moving from lib/Target and lib/CodeGen
- [LLVMdev] moving from lib/Target and lib/CodeGen
- [LLVMdev] Move TargetRegisterInfo and TargetInstrInfo into libCodeGen
- [LLVMdev] Move TargetRegisterInfo and TargetInstrInfo into libCodeGen