lenary added inline comments.
================ Comment at: llvm/lib/LTO/LTOBackend.cpp:151 + TargetMachine::initTargetOptions(M, Conf.Options); + ---------------- tejohnson wrote: > 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. Thank you! It is useful to have this restriction explicit :) 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
