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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
