[PATCH] D72624: [WIP] TargetMachine Hook for Module Metadata

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

>From an LTO perspective, this seems fine for the reasons we discussed here. I looked through the patch and have a few comments.



================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:818
+  if (TM) {
+    TM->initializeOptionsWithModuleMetadata(*TheModule);
     TheModule->setDataLayout(TM->createDataLayout());
----------------
Also needed in EmitAssemblyWithNewPassManager. Maybe it should be moved into CreateTargetMachine which would cover both cases.

I'm not sure if this was already discussed - but is there a reason why this can't be done in Target::createTargetMachine()? Is it not possible to ensure it is called once we have the Module available and pass that in? That would centralize this handling and seems cleaner overall.


================
Comment at: llvm/include/llvm/Target/TargetMachine.h:157
+  const DataLayout createDataLayout() const {
+    OptionsCanBeInitalizedFromModule = false;
+    return DL;
----------------
Do you want to also ensure that createDataLayout is only called iff initializeOptionsWithModuleMetadata was previously called? That would need to make this a tri-state, or use 2 bools. Then you could assert here that the other routine was already called at this point, which would help avoid missing calls (like the one I pointed out above), possibly due to future code drift.


================
Comment at: llvm/include/llvm/Target/TargetMachine.h:192
+  virtual void
+  setTargetOptionsWithModuleMetadata(const Module &M LLVM_ATTRIBUTE_UNUSED) {}
+
----------------
Should this be private so that it can only be called via initializeOptionsWithModuleMetadata?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72624