[Lldb-commits] [PATCH] D72624: [WIP] TargetMachine Hook for Module Metadata

[Sam Elliott via Phabricator via lldb-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 <https://reviews.llvm.org/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