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