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

khchen added a comment. Herald added a reviewer: herhut. Herald added subscribers: frgossen, grosul1, Joonsoo. Hi @lenary, I added a PoC patch D78035 to complete ThinLTO based on this patch. There is also a missed hook in `ParallelCG.cpp` @@ -28,6 +28,7 @@

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

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);

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

lenary added a comment. One thing to note about my patch, above: I have not made the TargetMachine DataLayout non-const, but I wanted to ensure that this might be possible in future, which is why calling `initializeOptionsWithModuleMetadata` must be done before the first call to

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

tejohnson added a comment. In D72624#1820598 , @dblaikie wrote: > (just a general comment that this code review should be only in service of > the design discussion happening on llvm-dev - please don't approve/commit > this without closing out the

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

dblaikie added a comment. (just a general comment that this code review should be only in service of the design discussion happening on llvm-dev - please don't approve/commit this without closing out the design discussion there if there are actionable conclusions from this review)

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

tejohnson added a comment. In D72624#1820281 , @lenary wrote: > In D72624#1817464 , @tejohnson wrote: > > > > > > Thank you for your feedback! It has been very helpful. > > > I'm not sure if

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

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 61807 tests passed, 0 failed and 781 were skipped. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts

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

lenary updated this revision to Diff 238053. lenary added a comment. Herald added subscribers: mgorny, MatzeB. Address some review feedback. This patch remains incomplete with regards to module flags and ThinLTO. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

lenary marked 2 inline comments as done. lenary added a comment. In D72624#1817464 , @tejohnson wrote: > Thank you for your feedback! It has been very helpful. > I'm not sure if ThinLTOCodeGenerator.cpp and LTOBackend.cpp were > intentionally left out

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

khchen added a comment. In D72624#1818605 , @khchen wrote: > I think putting the resetTargetDefaultOptions after instance of TargetMachine > is too late. > for example: > ppc >

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

khchen added a comment. I think putting the resetTargetDefaultOptions after instance of TargetMachine is too late. for example: ppc and mips

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

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

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

lenary created this revision. lenary added reviewers: tejohnson, dblaikie, khchen, dsanders, echristo. Herald added subscribers: llvm-commits, lldb-commits, cfe-commits, liufengdb, herhut, lucyrfox, mgester, arpith-jacob, csigg, nicolasvasilache, antiagainst, shauheen, burmako, jpienaar,