> The idea is fantastic, but I found your implementation burdensome. > > Bringing a TargetMachine dependency into IRGen, even if it's a "soft" > dependency is best avoided because the clear layering between IRGen and > CodeGen is LLVM's stated foremost design goal. > > The extraction of CreateTargetMachine() into a long list of parameters in > your patch also left something to be desired and we should be able to avoid > it. > > >> >> It might be possible to still refactor things to avoid the >> duplication, but for now this should at least make sure both projects >> stay in sync. > > > I've attached my take on your idea, implementing it in a net 8 LoC. > > In my patch, the verification is done at IRGen so there's no runtime > penalty, and moreover no change in layering.
Interesting. I started coding with the idea of removing the string from clang and using the ones in llvm at their current location. I got there and then went back to just asserting, but never looked back if there was a better spot to put the assert. I really like your patch. Some some small comments. * Making TM a member of EmitAssemblyHelper is a nice independent improvement. Please just commit that right away :-) + if (!TM) TM.reset(CreateTargetMachine(UsesCodeGen)); Using two lines seems to be the more current stile (what clang-format uses). + assert(TDesc.empty() || !AsmHelper.TM || This is failing on test/Frontend/ir-support-codegen.ll. It seems like the assert found a real bug. The TargetInfo is created once with // Create the target instance. setTarget(TargetInfo::CreateTargetInfo(getDiagnostics(), &getTargetOpts())); but then CodeGenAction will simply parse the module and use the triple in it. Given what a corner case this is, it might be reasonable to just pass an empty string in CodeGenAction.cpp:424 or maybe patch the Triple in the module. After all, clang always creates a single TargetInfo based on what is passed to -triple (or the system default), in cannot really handle an IR file with a different one. > Even with this verification done, we should continue to investigate options > for sharing the layouts and getting them threaded through the frontend. Two > copies are one too many. Agreed. I just don't promise I will have the cycles to do it right now. The assertion gives me enough confidence to add the features I need to DataLayout. Cheers, Rafael _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
