tejohnson added inline comments.
================ Comment at: llvm/lib/LTO/LTOBackend.cpp:151 + TargetMachine::initTargetOptions(M, Conf.Options); + ---------------- lenary wrote: > tejohnson wrote: > > This is going to be problematic. The Conf is a reference to the Config > > object saved on the LTO class instance shared by all backend invocations > > (the regular LTO module if one exists and any ThinLTO modules). They will > > end up clobbering each other's values here - although from the assert in > > initTargetOptions I see they are required to all have the same value > > anyway. Still, it is not good as the assert may actually be missed with > > unlucky interference between the threads. The Config object here should > > really be marked const, let me see if I can change that. > > > > You could make a copy of the Config here, but that essentially misses the > > assertion completely. > > > > A better way to do this would be in LTO::addModule, which is invoked > > serially to add each Module to the LTO object. > > > > However - this misses other places that invoke createTargetMachine - should > > other places be looking at this new module flag as well? One example I can > > think of is the old LTO API (*LTOCodeGenerator.cpp files), used by linkers > > such as ld64 and some other proprietary linkers and the llvm-lto testing > > tool. But I have no idea about other invocations of createTargetMachine. > > > > Note that changes to LTO.cpp/LTOBackend.cpp (the new LTO API) needs some > > kind of llvm-lto2 based test. > Thank you for this feedback. > > I've been looking at how to add an overridable TargetMachine hook which is > not dissimilar to this static function, but is overridable by TargetMachine > subclasses. It sounds like this approach will also not work (unless the > TargetMachine is allowed to update its (Default)Options in LTO without > issue). > > I am hoping to get a patch out today for review (which does not include the > RISC-V specific parts of this patch, and only includes a default empty > implementation), but I imagine it will remain unsatisfactory for LTO for the > same reasons this is. Presumably you could still do the same thing I'm suggesting here - validate and aggregate the value across modules in LTO::addModule. Then your hook would just check the aggregated setting on the Config. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72245/new/ https://reviews.llvm.org/D72245 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
