Page MenuHomePhabricator

[RISCV] enable LTO support, pass some options to linker.
ClosedPublic

Authored by khchen on Sep 10 2019, 11:00 AM.

Details

Summary
  1. enable LTO need to pass target feature and abi to LTO code generation RISCV backend need the target feature to decide which extension used in code generation.
  2. move getTargetFeatures to CommonArgs.h and add ForLTOPlugin flag
  3. add general tools::getTargetABI in CommonArgs.h because different target uses different way to get the target ABI.

Diff Detail

Event Timeline

khchen created this revision.Sep 10 2019, 11:00 AM

Can you add a test for riscv64-unknown-linux-gnu? I think RISCVToolchain.cpp is only used by the riscv64-unknown-elf target, but I could be wrong.

khchen updated this revision to Diff 219941.EditedSep 12 2019, 9:39 AM

@lenary Yes, you are right. add LTO support for Linux platform.

MaskRay added inline comments.Sep 12 2019, 9:53 AM
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
387 ↗(On Diff #219941)

gold doesn't support RISC-V, does it?

kito-cheng added inline comments.Sep 12 2019, 1:46 PM
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
387 ↗(On Diff #219941)

Gold doesn't support RISC-V , but ld.bfd supported same plugin API, so this made clang can run LTO with ld.bfd.

khchen added inline comments.Sep 12 2019, 5:24 PM
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
387 ↗(On Diff #219941)

I reference AddGoldPlugin to naming this function, is it confusing?

MaskRay added inline comments.Sep 12 2019, 11:18 PM
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
414 ↗(On Diff #219941)

The single-dash form of mattr: -plugin-opt=-mattr= should be preferred. --mattr is rare.

clang/lib/Driver/ToolChains/Arch/RISCV.h
26 ↗(On Diff #219941)

The description should probably mention why clang::driver:tools::AddGoldPlugin is insufficient.

khchen updated this revision to Diff 220214.Sep 14 2019, 8:03 AM
khchen edited the summary of this revision. (Show Details)

This commit is inspired by @MaskRay's suggestion, I think maybe fix the insufficient of clang::driver:tools::AddGoldPlugin is good choose.

I have some nits about explicit comments for arguments and default argument values for backwards compatibility. Other than that, it looks like a nice code cleanup.

clang/lib/Driver/ToolChains/Clang.cpp
6110

This needs /*ForAS=*/true, /*ForLTOPlugin=*/false) so it's clear what the booleans are for. Same for other calls to getTargetFeatures.

clang/lib/Driver/ToolChains/CommonArgs.h
127

The final argument here should probably default to false, so that out-of-tree backends do not break immediately when this patch lands.

khchen updated this revision to Diff 220668.Sep 18 2019, 7:58 AM

Thanks @lenary, I fixed it.

lenary accepted this revision.Tue, Oct 8, 7:21 AM

Nice, LGTM

This revision is now accepted and ready to land.Tue, Oct 8, 7:21 AM
khchen updated this revision to Diff 224821.Mon, Oct 14, 2:13 AM

rebase master,
https://reviews.llvm.org/D66003 changed the riscv::getRISCVTargetFeatures interface.

@khchen do you need me to commit this for you?

@khchen do you need me to commit this for you?

Yes, please help me, thanks.

This revision was automatically updated to reflect the committed changes.

@khchen Thanks for your patch! It is now landed.