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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
