Page MenuHomePhabricator

[PoC][RISCV] enable LTO/ThinLTO on RISCV
Needs ReviewPublic

Authored by khchen on Apr 13 2020, 9:54 AM.

Details

Reviewers
lenary
tejohnson
Summary

This patch enable LTO/ThinLTO on RISCV based on TargetMachine Hook in D72624.

Diff Detail

Event Timeline

khchen created this revision.Apr 13 2020, 9:54 AM
MaskRay added inline comments.Apr 13 2020, 12:23 PM
llvm/test/LTO/RISCV/mabi.ll
6

llvm-lto is for the old LTO API and llvm-lto2 is for the new API. The file does not test llvm-lto2 for full LTO.

I am not clear about how tests are organized in this directory. Hope @tejohnson can clarify... and point out the ideal directory structure..

tejohnson added inline comments.Apr 13 2020, 3:45 PM
llvm/lib/LTO/LTO.cpp
1029 ↗(On Diff #257010)

We definitely don't want to be doing this. We by design do not parse IR of ThinLTO modules during the LTO link, only in the ThinLTO backends. This is what helps keep ThinLTO fast. I'm not sure what the value of reading it in only one of potentially many ThinLTO modules is anyway? If you want to detect mismatches between ThinLTO modules, it will have to either be done naturally in the ThinLTO backends when we (selectively) merge IR there (i.e. caught by the IRMover). Otherwise, if you want to do it during the thin link, then you will need to record this information in the ThinLTO summary. See my comment here (the two big paragraphs towards the end of my comment): https://reviews.llvm.org/D72624#1820580

llvm/test/LTO/RISCV/mabi.ll
6

As @MaskRay notes, llvm-lto tests the "old" LTO API, e.g. ThinLTOCodeGenerator.cpp and LTOCodeGenerator.cpp. The code in LTO.cpp for example is the "new" linker-resolution based LTO, which is tested by llvm-lto2.

The Resolution subdirectory of llvm/test/LTO was originally meant for testing the "new" resolution based LTO. But we have subsequently added a lot of new LTO based testing via llvm-lto2 in the other directories, for things that we might want to test for both APIs (as they are both still in active use by different linkers).

This is a good place to add this test, since this is based on the support in D72624 which adds the hook to both the old and new LTO APIs. Ideally you should test both regular and ThinLTO with both llvm-lto and llvm-lto2 to get all combinations.

khchen marked 2 inline comments as done.Apr 14 2020, 1:56 AM
khchen added inline comments.
llvm/lib/LTO/LTO.cpp
1029 ↗(On Diff #257010)

@tejohnson Thanks, it has been very helpful! I would try summary info approach to avoid to parse IR.

Different target-abi flag generates different elf objects and they can not linked together, but runRegularLTO generates empty object from CombinedModule (empty module) without any target-abi module flag, it means in some case this CombinedModule object can not link with other input module.
This is why I copy module flag from input to CombinedModule.
In LTO, the only legal way is all input modules have same target-abi flag, so CombinedModule does not care where is the target-abi flag value come from.

llvm/test/LTO/RISCV/mabi.ll
6

@MaskRay Thanks for your reminder.

@tejohnson

Ideally you should test both regular and ThinLTO with both llvm-lto and llvm-lto2 to get all combinations.

Do you mean if I claim RISCV supports thinLTO, ideally I need to add more testcases like X86 in test/LTO and test/ThinLTO/X86?
I saw there are just few testcase in test/LTO/ARM, for example lto-linking-metadata.ll" also tests llvm-lto and llvm-lto2, so I did the similar thing in RISCV...

tejohnson added inline comments.Apr 14 2020, 10:40 AM
llvm/lib/LTO/LTO.cpp
1029 ↗(On Diff #257010)

In the ThinLTO case, as you note the CombinedModule is empty. So why does the target-abi matter? Does the linker complain? If so, we should probably just suppress emission of this empty object file. I think it is always emitted because that helps in some build systems, like ours, which want to know a priori all possible output files, but perhaps we could put this under an option. But I'd like to understand whether this is resulting in a link failure first.

llvm/test/LTO/RISCV/mabi.ll
6

Not necessarily now, but I would imagine if RISCV ThinLTO is supported, then eventually more RISCV-specific test cases will be added. For now something simple is probably ok, to test the basic cases. You can add the llvm-lto and llvm-lto2 testing into the same .ll test - that's what I meant. Test all 4 combinations (new/old LTO api and regular/thin LTO), rather than just the two you have now (regular+old, thin+new).

khchen marked 2 inline comments as done.Apr 14 2020, 7:11 PM
khchen added inline comments.
llvm/lib/LTO/LTO.cpp
1029 ↗(On Diff #257010)

Yes, linker emits an error, this situation is similar to linking ARM's softfp object with hardfp object, but RISCV have more combinations and flexibility with abi in the same platform.

ps. In other targets (ex. ARM, MIPS), the abi (target-abi) info for a file is derived from target triple, -mcpu or -mabi, but in RISC-V, abi is only derived from -mabi or -mattr (and both are encoded in IR now). It's why ARM doesn't have this issue.
BTW, GCC LTO also encodes the abi info into the ELF.

llvm/test/LTO/RISCV/mabi.ll
6

Got it, thanks.

khchen planned changes to this revision.Apr 19 2020, 8:36 PM
khchen marked an inline comment as done.

I plan to suppress to emit empty module or avoid ld to link empty module's object file.

llvm/lib/LTO/LTO.cpp
1029 ↗(On Diff #257010)

Sorry, I didn't make it clear enough. This linking error only happen in gold ld (I had not tested lld yet) due to the file list for linking includes empty CombinedModule object.

ps. CombinedModule is not always empty in mixed-mode LTO (ex. gold/X86/mixed_lto.ll). It means my original patch has bug. :S

I plan to suppress to emit empty module or avoid ld to link empty module's object file.

This will need to be dependent on linker options. Our build system relies on always getting this object file when the -Wl,-plugin-opt,obj-path=${file} (gold-plugin.cpp). or -Wl,-lto-obj-path=${file} (lld) options are passed. Presumably you could add a boolean flag to the lto::Config to communicate whether this option was specified and use that to control creation of the output even when it is an empty module.

If you create a new patch for this work please make sure I am a reviewer so I can make sure we aren't broken.

llvm/lib/LTO/LTO.cpp
1029 ↗(On Diff #257010)

This linking error only happen in gold ld (I had not tested lld yet) due to the file list for linking includes empty CombinedModule object.

Interesting. The other option then is to figure out how lld is ignoring this empty file and add similar logic into gold-plugin.cpp. Basically you would want to avoid the call to add_input_file for any empty objects (this is the callback into gold to add the object files to be linked). At the point where we call that we just have the file name and a boolean indicating whether it should be cleaned up or not after the link. You would need to do some kind of restructuring or possibly make the Files array a tuple with another bool flag indicating whether the obj file buffer was empty.

ps. CombinedModule is not always empty in mixed-mode LTO (ex. gold/X86/mixed_lto.ll). It means my original patch has bug. :S

Right, in that case it gets the ABI flag naturally from the regular LTO modules. It's not even just mixed lto mode, we can have split hybrid LTO modules in some cases for CFI and WPD (one regular LTO module and one ThinLTO module within one .o bitcode file), where you will end up with a non-empty combined module when building with ThinLTO.

khchen updated this revision to Diff 261132.Apr 29 2020, 10:48 PM
khchen retitled this revision from [PoC][RISCV] enable LTO/ThinLTO on RISCV to [PoC][RISCV] enable LTO/ThinLTO on RISCV.
khchen edited the summary of this revision. (Show Details)

rebase on D78988

Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2020, 10:48 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tejohnson added inline comments.Apr 30 2020, 1:26 PM
llvm/test/LTO/RISCV/mabi-invalid.ll
11

Here's another case where the regular LTO testing is using the old LTO API (via llvm-lto) and ThinLTO is using the new LTO API (via llvm-lto2). As noted earlier, all 4 combinations should be tested.

llvm/test/tools/gold/RISCV/mixed_lto.ll
20

There's a typo in the input name "miaxed_lto.ll". If the test worked despite that then it probably needs some improvement.

khchen updated this revision to Diff 262322.May 6 2020, 2:22 AM

update testcases