[PATCH] D72245: [PoC][RISCV][LTO] Pass target-abi via module flag metadata

[Teresa Johnson via Phabricator via cfe-commits]

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