[Lldb-commits] [PATCH] D72624: [WIP] TargetMachine Hook for Module Metadata

[Teresa Johnson via Phabricator via lldb-commits]

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72624