[PoC][RISCV][LTO] Pass target-abi via module flag metadata

[khchen]

Summary

1. Support ABI checking with per function target-features

if users don't specific -mattr, the default target-feature come from IR attribute.

2. Enable LTO for RISCV
3. add TargetMachine::initTargetOptions to overwirte MCOptions.ABINameaccording to module info

[kito-cheng]

Seems like this patch mixed with LTO related changes? Could you clean it up?

[khchen]

remote LTO related code.
this PoC include D70837 patch for generate correct code.

[khchen]

Hi @efriedma
Could you please guide me and review this PoC?
or take a look at this [[ take a look at | maillist ]] thread?
Thank you!

[tejohnson]

llvm/lib/LTO/LTOBackend.cpp

151

This is going to be problematic. The Conf is a reference to the Config object saved on the LTO class instance shared by all backend invocations (the regular LTO module if one exists and any ThinLTO modules). They will end up clobbering each other's values here - although from the assert in initTargetOptions I see they are required to all have the same value anyway. Still, it is not good as the assert may actually be missed with unlucky interference between the threads. The Config object here should really be marked const, let me see if I can change that.

You could make a copy of the Config here, but that essentially misses the assertion completely.

A better way to do this would be in LTO::addModule, which is invoked serially to add each Module to the LTO object.

However - this misses other places that invoke createTargetMachine - should other places be looking at this new module flag as well? One example I can think of is the old LTO API (*LTOCodeGenerator.cpp files), used by linkers such as ld64 and some other proprietary linkers and the llvm-lto testing tool. But I have no idea about other invocations of createTargetMachine.

Note that changes to LTO.cpp/LTOBackend.cpp (the new LTO API) needs some kind of llvm-lto2 based test.

[lenary]

llvm/lib/LTO/LTOBackend.cpp

151

Thank you for this feedback.

I've been looking at how to add an overridable TargetMachine hook which is not dissimilar to this static function, but is overridable by TargetMachine subclasses. It sounds like this approach will also not work (unless the TargetMachine is allowed to update its (Default)Options in LTO without issue).

I am hoping to get a patch out today for review (which does not include the RISC-V specific parts of this patch, and only includes a default empty implementation), but I imagine it will remain unsatisfactory for LTO for the same reasons this is.

[tejohnson]

llvm/lib/LTO/LTOBackend.cpp

151

Presumably you could still do the same thing I'm suggesting here - validate and aggregate the value across modules in LTO::addModule. Then your hook would just check the aggregated setting on the Config.

[lenary]

llvm/lib/LTO/LTOBackend.cpp

151

D72624 is the patch I have prepared, noting I haven't had time to implement the aggregation yet, which suggests that patch's approach is too general.

[tejohnson]

llvm/lib/LTO/LTOBackend.cpp

151

FYI I committed a change to make the Config object passed down to the backends a const reference in d0aad9f56e1588effa94b15804b098e6307da6b4.

[lenary]

llvm/lib/LTO/LTOBackend.cpp

151

Thank you! It is useful to have this restriction explicit :)

[khchen]

I'm not working on RISC-V now and please reference https://reviews.llvm.org/D132843#3770454 to see the follow-up work.