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

[Kuan Hsu Chen (Zakk) via Phabricator via cfe-commits]

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 @@ static void codegen(Module *M, llvm::raw_pwrite_stream &OS,
   function_ref()> TMFactory,
   CodeGenFileType FileType) {
 std::unique_ptr TM = TMFactory();
  +  TM->initializeOptionsWithModuleMetadata(*M);
 legacy::PassManager CodeGenPasses;
 if (TM->addPassesToEmitFile(CodeGenPasses, OS, nullptr, FileType))
   report_fatal_error("Failed to setup codegen");

I'm not sure passing Module in Target::createTargetMachine() is a good or not,
because maybe the module flag will changed in next time on JIT compilation 
scenario?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Teresa Johnson via Phabricator via cfe-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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam Elliott via Phabricator via cfe-commits]

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 `createDataLayout`. At the moment, the RISC-V ABI data 
layouts are based only on riscv32 vs riscv64, not the `target-abi` metadata 
(all riscv32 ABIs use the same data layout, all riscv64 ABIs use the same data 
layout), but I know Mips has more complex logic for computing their data layout 
based on their ABI.

In D72624#1820580 , @tejohnson wrote:

> The ThinLTO "link", which is where the modules are added serially, does not 
> read IR, only the summaries, which are linked together into a large index 
> used to drive ThinLTO whole program analysis. So you can't really read the 
> module flags directly during addModule, they need to be propagated via the 
> summary flags. The ThinLTO backends which are subsequently fired off in 
> parallel do read IR. In those backends, depending on the results of the 
> ThinLTO analysis phase, we may use IRMover to link in ("import) functions 
> from other modules. At that point, the module flags from any modules that 
> backend is importing from will be combined and any errors due to conficting 
> values will be issued.


This has been a very helpful explanation of ThinLTO.

> Thinking through this some more, rather than attempting to fully validate the 
> consistency of the module flags across all modules in ThinLTO mode, just rely 
> on some checking when we merge subsections of the IR in the ThinLTO backends 
> during this importing, which will happen automatically. This is presumably 
> where the checking is desirable anyway (in terms of the cases you are most 
> interested in catching with ThinLTO, because the IR is getting merged). Note 
> that unlike in the full LTO case, where the IR is merged before you create 
> the TM, in the ThinLTO case the TM will be created before any of this 
> cross-module importing (partial IR merging), so with your patch presumably it 
> will just use whatever module flag is on that original Module for it's 
> corresponding ThinLTO backend. But since it sounds like any difference in 
> these module flags is an error, it will just get flagged a little later but 
> not affect how the TM is set up in the correct case. Does that sound 
> reasonable?

That does sound reasonable. I want errors to be reported, which it sounds like 
will happen, even if it is only "lazily" when using ThinLTO.

At some point in the future the ThinLTO summaries might want to gain knowledge 
of the module flags, which would help with eager error reporting (i.e., ThinLTO 
telling the user that two modules are incompatible before it does any 
analysis), but I think that is a step too far for this patch.

I shall look at making a patch with the RISC-V specific behaviour that @khchen 
and I intend implement on top of this, and then running more tests (including 
doing llvm test-suite runs with each kind of LTO enabled).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Teresa Johnson via Phabricator via cfe-commits]

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 design discussion there if there are actionable 
> conclusions from this review)


Got it, I will just look at from the LTO perspective. The target ABI specifics 
I haven't followed very closely and are not really my expertise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via Phabricator via cfe-commits]

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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Teresa Johnson via Phabricator via cfe-commits]

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 ThinLTOCodeGenerator.cpp and LTOBackend.cpp were 
> > intentionally left out due to the LTO concerns mentioned in the description?
>
> Mostly left out because I wasn't sure how to attack them. I've got an update 
> to this patch which I'm testing right now and it looks much better. I will 
> post that imminently.
>
> > 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 
> > , but neither would you get the proper 
> > aggregation/checking across ThinLTO'ed modules.
>
> Ok, right, so I think I know what else this patch needs to do. At the moment, 
> I think the `ModFlagBehavior` for module flags are not being checked during 
> ThinLTO. I think this is something that has to be checked for compatibility 
> in `ThinLTOCodeGenerator::addModule` (like the triple is checked for 
> compatibility).


And LTO::addModule (just to add confusion, there are two LTO APIs, 
ThinLTOCodeGenerator is the old one and LTO is the new one, the latter being 
used by lld and the gold plugin).

I had mentioned using LTO::addModule to do the checking in the other patch, but 
there is a complication I should mention:

> I see that the checking behaviour is in `IRMover`, but I don't think ThinLTO 
> uses that, and I don't feel familiar enough with ThinLTO to be sure.

The ThinLTO "link", which is where the modules are added serially, does not 
read IR, only the summaries, which are linked together into a large index used 
to drive ThinLTO whole program analysis. So you can't really read the module 
flags directly during addModule, they need to be propagated via the summary 
flags. The ThinLTO backends which are subsequently fired off in parallel do 
read IR. In those backends, depending on the results of the ThinLTO analysis 
phase, we may use IRMover to link in ("import) functions from other modules. At 
that point, the module flags from any modules that backend is importing from 
will be combined and any errors due to conficting values will be issued.

Thinking through this some more, rather than attempting to fully validate the 
consistency of the module flags across all modules in ThinLTO mode, just rely 
on some checking when we merge subsections of the IR in the ThinLTO backends 
during this importing, which will happen automatically. This is presumably 
where the checking is desirable anyway (in terms of the cases you are most 
interested in catching with ThinLTO, because the IR is getting merged). Note 
that unlike in the full LTO case, where the IR is merged before you create the 
TM, in the ThinLTO case the TM will be created before any of this cross-module 
importing (partial IR merging), so with your patch presumably it will just use 
whatever module flag is on that original Module for it's corresponding ThinLTO 
backend. But since it sounds like any difference in these module flags is an 
error, it will just get flagged a little later but not affect how the TM is set 
up in the correct case. Does that sound reasonable?

> The update to my patch will not address this part of ThinLTO.

I'll take a look through your patch later today or tomorrow, but it may be just 
fine from the above perspective.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

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 
: 
diff.json 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam Elliott via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D72624/new/

https://reviews.llvm.org/D72624

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp
  
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptExpressionOpts.cpp
  llvm/docs/tutorial/MyFirstLanguageFrontend/LangImpl08.rst
  llvm/examples/Kaleidoscope/Chapter8/toy.cpp
  llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
  llvm/include/llvm/Target/TargetMachine.h
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/lib/LTO/ThinLTOCodeGenerator.cpp
  llvm/lib/Target/TargetMachine.cpp
  llvm/lib/Target/TargetMachineC.cpp
  llvm/tools/llc/llc.cpp
  llvm/tools/llvm-isel-fuzzer/llvm-isel-fuzzer.cpp
  llvm/tools/llvm-opt-fuzzer/llvm-opt-fuzzer.cpp
  llvm/tools/opt/opt.cpp
  llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp
  llvm/unittests/CodeGen/GlobalISel/GISelMITest.h
  llvm/unittests/ExecutionEngine/Orc/CMakeLists.txt
  llvm/unittests/ExecutionEngine/Orc/RemoteObjectLayerTest.cpp
  llvm/unittests/MI/LiveIntervalTest.cpp
  mlir/lib/Conversion/GPUToCUDA/ConvertKernelFuncToCubin.cpp
  mlir/lib/ExecutionEngine/ExecutionEngine.cpp

Index: mlir/lib/ExecutionEngine/ExecutionEngine.cpp
===
--- mlir/lib/ExecutionEngine/ExecutionEngine.cpp
+++ mlir/lib/ExecutionEngine/ExecutionEngine.cpp
@@ -110,8 +110,9 @@
   }
   std::unique_ptr machine(
   target->createTargetMachine(targetTriple, "generic", "", {}, {}));
-  llvmModule->setDataLayout(machine->createDataLayout());
   llvmModule->setTargetTriple(targetTriple);
+  machine->initializeOptionsWithModuleMetadata(*llvmModule);
+  llvmModule->setDataLayout(machine->createDataLayout());
   return false;
 }
 
Index: mlir/lib/Conversion/GPUToCUDA/ConvertKernelFuncToCubin.cpp
===
--- mlir/lib/Conversion/GPUToCUDA/ConvertKernelFuncToCubin.cpp
+++ mlir/lib/Conversion/GPUToCUDA/ConvertKernelFuncToCubin.cpp
@@ -139,6 +139,7 @@
   }
 
   // Set the data layout of the llvm module to match what the ptx target needs.
+  targetMachine->initializeOptionsWithModuleMetadata(llvmModule);
   llvmModule.setDataLayout(targetMachine->createDataLayout());
 
   auto ptx = translateModuleToPtx(llvmModule, *targetMachine);
Index: llvm/unittests/MI/LiveIntervalTest.cpp
===
--- llvm/unittests/MI/LiveIntervalTest.cpp
+++ llvm/unittests/MI/LiveIntervalTest.cpp
@@ -50,8 +50,10 @@
 }
 
 std::unique_ptr parseMIR(LLVMContext &Context,
-legacy::PassManagerBase &PM, std::unique_ptr &MIR,
-const LLVMTargetMachine &TM, StringRef MIRCode, const char *FuncName) {
+ legacy::PassManagerBase &PM,
+ std::unique_ptr &MIR,
+ LLVMTargetMachine &TM, StringRef MIRCode,
+ const char *FuncName) {
   SMDiagnostic Diagnostic;
   std::unique_ptr MBuffer = MemoryBuffer::getMemBuffer(MIRCode);
   MIR = createMIRParser(std::move(MBuffer), Context);
@@ -62,6 +64,7 @@
   if (!M)
 return nullptr;
 
+  TM.initializeOptionsWithModuleMetadata(*M);
   M->setDataLayout(TM.createDataLayout());
 
   MachineModuleInfoWrapperPass *MMIWP = new MachineModuleInfoWrapperPass(&TM);
Index: llvm/unittests/ExecutionEngine/Orc/RemoteObjectLayerTest.cpp
===
--- llvm/unittests/ExecutionEngine/Orc/RemoteObjectLayerTest.cpp
+++ llvm/unittests/ExecutionEngine/Orc/RemoteObjectLayerTest.cpp
@@ -93,6 +93,7 @@
 
   LLVMContext Ctx;
   ModuleBuilder MB(Ctx, TM->getTargetTriple().str(), "TestModule");
+  TM->initializeOptionsWithModuleMetadata(*MB.getModule());
   MB.getModule()->setDataLayout(TM->createDataLayout());
   auto *Main = MB.createFunctionDecl(
   FunctionType::get(Type::getInt32Ty(Ctx),
Index: llvm/unittests/ExecutionEngine/Orc/CMakeLists.txt
===
--- llvm/unittests/ExecutionEngine/Orc/CMakeLists.txt
+++ llvm/unittests/ExecutionEngine/Orc/CMakeLists.txt
@@ -8,6 +8,7 @@
   Passes
   RuntimeDyld
   Support
+  Target
   native
   )
 
Index: llvm/unittests/CodeGen/GlobalISel/GISelMITest.h
===
--- llvm/unittests/CodeGen/GlobalISel/GISelMITest.h
+++ llvm/unittests/CodeGen/GlobalISel/GISelMITest.h
@@ -70,8 +70,8 @@
 
 static std::unique_ptr parseMIR(LLVMContext &Context,
 std::unique_ptr &MIR,
-const TargetMachine &TM,
-

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

[Sam Elliott via Phabricator via cfe-commits]

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 due to the LTO concerns mentioned in the description?

Mostly left out because I wasn't sure how to attack them. I've got an update to 
this patch which I'm testing right now and it looks much better. I will post 
that imminently.

> 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 
> , but neither would you get the proper 
> aggregation/checking across ThinLTO'ed modules.

Ok, right, so I think I know what else this patch needs to do. At the moment, I 
think the `ModFlagBehavior` for module flags are not being checked during 
ThinLTO. I think this is something that has to be checked for compatibility in 
`ThinLTOCodeGenerator::addModule` (like the triple is checked for 
compatibility).

I see that the checking behaviour is in `IRMover`, but I don't think ThinLTO 
uses that, and I don't feel familiar enough with ThinLTO to be sure.

The update to my patch will not address this part of ThinLTO.




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.

tejohnson wrote:
> 
> Looks like DefaultOptions is const, so override methods wouldn't be able to 
> change it.
I contemplated making `DefaultOptions` non-const, but the truth is lots of 
subclasses of TargetMachine set new values to `Options` in the subclass 
initializers.

So the intention now is that the hook can just set more values on `Options`.



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;

tejohnson wrote:
> 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.
Yeah, the idea is that a target-specific subclass will override this method, 
and extract module flags from M, which they can use to set values in `Options`.

In the case of RISC-V, the RISCVTargetMachine will use the `target-abi` module 
flag to set `Options.MCOptions.ABIName`. I hope that it might also be used by 
other backends like Mips, but I think their case is already handled by LLVM at 
the moment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kuan Hsu Chen (Zakk) via Phabricator via cfe-commits]

khchen added a comment.

In D72624#1818605 , @khchen wrote:

> I think putting the resetTargetDefaultOptions after instance of TargetMachine 
> is too late.
>  for example: 
>  ppc 
> 
>  and mips 
> 
>  compute the TargetABI in TargetMachine constructor. In addition , mips 
> 
>  compute the DataLayout with target ABI in TargetMachine constructor.


Sorry, I didn't notice the resetTargetDefaultOptions is a virtual function, so 
the backend need to care this issue themselves if they want take this approach. 
I think this approach is better than D72245 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kuan Hsu Chen (Zakk) via Phabricator via cfe-commits]

khchen added a comment.

I think putting the resetTargetDefaultOptions after instance of TargetMachine 
is too late.
for example: 
ppc 

 and mips 

 compute the TargetABI in TargetMachine constructor. In addition , mips 

 compute the DataLayout with target ABI in TargetMachine constructor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Teresa Johnson via Phabricator via cfe-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 
, 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam Elliott via Phabricator via cfe-commits]

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, rriddle, mehdi_amini, dang, dexonsmith, steven_wu, 
hiraditya.
Herald added projects: clang, LLDB, LLVM.

This patch attempts to add a target-overridable hook for allowing module 
metadata
to override TargetMachine options, especially during LTO.

The new hook is called `TargetMachine::resetTargetDefaultOptions` and takes the
`llvm::Module` so that the module metadata can be read with the API provided
by `llvm::Module`.

It is not clear that this patch handles LTO correctly at the moment.

This patch relates to D72245  and D72246 
 and the discussion in
http://lists.llvm.org/pipermail/llvm-dev/2020-January/138151.html


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72624

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp
  
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptExpressionOpts.cpp
  llvm/docs/tutorial/MyFirstLanguageFrontend/LangImpl08.rst
  llvm/examples/Kaleidoscope/Chapter8/toy.cpp
  llvm/include/llvm/Target/TargetMachine.h
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/lib/Target/TargetMachine.cpp
  llvm/lib/Target/TargetMachineC.cpp
  llvm/tools/llc/llc.cpp
  llvm/tools/llvm-isel-fuzzer/llvm-isel-fuzzer.cpp
  llvm/tools/llvm-opt-fuzzer/llvm-opt-fuzzer.cpp
  llvm/tools/opt/opt.cpp
  mlir/lib/Conversion/GPUToCUDA/ConvertKernelFuncToCubin.cpp
  mlir/lib/ExecutionEngine/ExecutionEngine.cpp

Index: mlir/lib/ExecutionEngine/ExecutionEngine.cpp
===
--- mlir/lib/ExecutionEngine/ExecutionEngine.cpp
+++ mlir/lib/ExecutionEngine/ExecutionEngine.cpp
@@ -110,8 +110,9 @@
   }
   std::unique_ptr machine(
   target->createTargetMachine(targetTriple, "generic", "", {}, {}));
-  llvmModule->setDataLayout(machine->createDataLayout());
   llvmModule->setTargetTriple(targetTriple);
+  machine->resetTargetDefaultOptions(*llvmModule);
+  llvmModule->setDataLayout(machine->createDataLayout());
   return false;
 }
 
Index: mlir/lib/Conversion/GPUToCUDA/ConvertKernelFuncToCubin.cpp
===
--- mlir/lib/Conversion/GPUToCUDA/ConvertKernelFuncToCubin.cpp
+++ mlir/lib/Conversion/GPUToCUDA/ConvertKernelFuncToCubin.cpp
@@ -142,6 +142,7 @@
   }
 
   // Set the data layout of the llvm module to match what the ptx target needs.
+  targetMachine->resetTargetDefaultOptions(llvmModule);
   llvmModule.setDataLayout(targetMachine->createDataLayout());
 
   auto ptx = translateModuleToPtx(llvmModule, *targetMachine);
Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -674,6 +674,8 @@
 
   std::unique_ptr TM(Machine);
 
+  TM->resetTargetDefaultOptions(*M);
+
   // Override function attributes based on CPUStr, FeaturesStr, and command line
   // flags.
   setFunctionAttributes(CPUStr, FeaturesStr, *M);
Index: llvm/tools/llvm-opt-fuzzer/llvm-opt-fuzzer.cpp
===
--- llvm/tools/llvm-opt-fuzzer/llvm-opt-fuzzer.cpp
+++ llvm/tools/llvm-opt-fuzzer/llvm-opt-fuzzer.cpp
@@ -123,6 +123,7 @@
   //
 
   M->setTargetTriple(TM->getTargetTriple().normalize());
+  TM->resetTargetDefaultOptions(*M);
   M->setDataLayout(TM->createDataLayout());
   setFunctionAttributes(TM->getTargetCPU(), TM->getTargetFeatureString(), *M);
 
Index: llvm/tools/llvm-isel-fuzzer/llvm-isel-fuzzer.cpp
===
--- llvm/tools/llvm-isel-fuzzer/llvm-isel-fuzzer.cpp
+++ llvm/tools/llvm-isel-fuzzer/llvm-isel-fuzzer.cpp
@@ -91,6 +91,7 @@
 
   // Set up the module to build for our target.
   M->setTargetTriple(TM->getTargetTriple().normalize());
+  TM->resetTargetDefaultOptions(*M);
   M->setDataLayout(TM->createDataLayout());
 
   // Build up a PM to do instruction selection.
Index: llvm/tools/llc/llc.cpp
===
--- llvm/tools/llc/llc.cpp
+++ llvm/tools/llc/llc.cpp
@@ -504,6 +504,9 @@
 TLII.disableAllFunctions();
   PM.add(new TargetLibraryInfoWrapperPass(TLII));
 
+  // Initialise target default options from module metadata
+  Target->resetTargetDefaultOptions(*M);
+
   // Add the target data from the target machine, if it exists, or the module.
   M->setDataLayout(Target->createDataLayout());
 
Index: llvm/lib/Target/TargetMachineC.cpp
===
--- llvm/lib/Target/TargetMachineC.cpp
+++ llvm/lib/Target/TargetMachineC.cpp
@@ -193,6 +193,7 @@
 
   std::string error;