tejohnson added inline comments.
================ Comment at: llvm/lib/LTO/LTOBackend.cpp:151 + TargetMachine::initTargetOptions(M, Conf.Options); + ---------------- lenary wrote: > 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. FYI I committed a change to make the Config object passed down to the backends a const reference in d0aad9f56e1588effa94b15804b098e6307da6b4. 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
