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 

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 

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.

  rG LLVM Github Monorepo



lldb-commits mailing list

Reply via email to