lenary added inline comments.
================ Comment at: llvm/lib/LTO/LTOBackend.cpp:151 + TargetMachine::initTargetOptions(M, Conf.Options); + ---------------- tejohnson wrote: > 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. D72624 is the patch I have prepared, noting I haven't had time to implement the aggregation yet, which suggests that patch's approach is too general. 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
