tejohnson added a comment.

I'm not sure if ThinLTOCodeGenerator.cpp and LTOBackend.cpp were intentionally 
left out due to the LTO concerns mentioned in the description?

Note if we are just passing in the Module and updating the TM based on that, it 
wouldn't hit the threading issue I mentioned in D72245 
<https://reviews.llvm.org/D72245>, but neither would you get the proper 
aggregation/checking across ThinLTO'ed modules.

Comment at: llvm/include/llvm/Target/TargetMachine.h:188
+  // `M.setTargetTriple(TM->getTargetTriple())` and before
+  // `M.setDataLayout(createDataLayout())`.
+  virtual void resetTargetDefaultOptions(const Module &M) const;
Is there a way to enforce this? Otherwise I foresee fragility. E.g. what if 
this method set a flag on the TM indicating that we have updated it properly 
from the Module, and TargetMachine::createDataLayout() asserted that this flag 
was set?

Comment at: llvm/lib/Target/TargetMachine.cpp:51
+// Override methods should only change DefaultOptions, and use this super
+// method to copy the default options into the current options.

Looks like DefaultOptions is const, so override methods wouldn't be able to 
change it.

Comment at: llvm/lib/Target/TargetMachine.cpp:53
+// method to copy the default options into the current options.
+void TargetMachine::resetTargetDefaultOptions(const Module &M) const {
+  Options = DefaultOptions;
Can you clarify how M will be used - will a follow on patch set the 
MCOptions.ABIName from the Module? Note in the meantime you will likely need to 
mark this with an LLVM_ATTRIBUTE_UNUSED.

  rG LLVM Github Monorepo



lldb-commits mailing list

Reply via email to