[WIP] TargetMachine Hook for Module Metadata

[lenary]

Summary

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

[tejohnson]

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.

llvm/include/llvm/Target/TargetMachine.h

194

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?

llvm/lib/Target/TargetMachine.cpp

52

Looks like DefaultOptions is const, so override methods wouldn't be able to change it.

54

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.

[khchen]

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

[khchen]

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 XXXXTargetMachine 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.

[lenary]

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.

llvm/lib/Target/TargetMachine.cpp

52

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.

54

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.

[lenary]

Address some review feedback.

This patch remains incomplete with regards to module flags and ThinLTO.

[merge_guards_bot]

Unit tests: pass. 61807 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[tejohnson]

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.

[dblaikie]

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

[tejohnson]

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.

[lenary]

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

[tejohnson]

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

clang/lib/CodeGen/BackendUtil.cpp

818

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.

llvm/include/llvm/Target/TargetMachine.h

157

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.

192

Should this be private so that it can only be called via initializeOptionsWithModuleMetadata?

[khchen]

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<std::unique_ptr<TargetMachine>()> TMFactory,
                     CodeGenFileType FileType) {
   std::unique_ptr<TargetMachine> 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?