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

[Sam Elliott via Phabricator via cfe-commits]

lenary added inline comments.


================
Comment at: llvm/lib/LTO/LTOBackend.cpp:151
 
+  TargetMachine::initTargetOptions(M, Conf.Options);
+
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72245/new/

https://reviews.llvm.org/D72245