This is an archive of the discontinued LLVM Phabricator instance.

[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.Oct 8 2019, 7:21 AM

Nice, LGTM

This revision is now accepted and ready to land.Oct 8 2019, 7:21 AM
khchen updated this revision to Diff 224821.Oct 14 2019, 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.

efriedma added inline comments.
clang/lib/Driver/ToolChains/CommonArgs.cpp
498

I don't think this change is right. In general, target features should be encoded in bitcode files. This allows compiling different files with different target features, and using runtime detection to only run certain codepaths. And it makes sure that we end up with a sane result if the user doesn't pass target feature flags on the link line.

Also, it probably isn't appropriate to make target-independent changes in a commit tagged [RISCV]; most people would assume a change marked like that doesn't have target-independent effects.

(Sorry about the delayed response to this review; I only just ran into this.)

khchen marked an inline comment as done.Nov 8 2019, 3:22 AM
khchen added inline comments.
clang/lib/Driver/ToolChains/CommonArgs.cpp
498

I don't think this change is right. In general, target features should be encoded in bitcode files. This allows compiling different files with different target features, and using runtime detection to only run certain codepaths. And it makes sure that we end up with a sane result if the user doesn't pass target feature flags on the link line.

I'm curious about your scenario, LTO will link two bitcodes file into one, so which target-features should be kept in final bitcode since different files have different target features? For example, one target features" is "+armv7-a" and another is "+armv8-a".

I guess maybe your case is they are same target-features in different files, but this patch will overwrite the encoded target-feature as default.

anyway, I agree with you. I found the target features does not encoded in bitcode files when enabling LTO in RISCV, I will fixed it and revert the target feature part, thanks.

Also, it probably isn't appropriate to make target-independent changes in a commit tagged [RISCV]; most people would assume a change marked like that doesn't have target-independent effects.

sorry, I will take care of it in next time.

(Sorry about the delayed response to this review; I only just ran into this.)

efriedma added inline comments.Nov 8 2019, 11:37 AM
clang/lib/Driver/ToolChains/CommonArgs.cpp
498

Target features are encoded into IR on a per-function basis, so we can keep both.

But yes, I'm more worried about the implicit expectation that the target flags are specified on the link line; this isn't reliably true with common build systems.

hans added a subscriber: hans.Nov 11 2019, 2:02 AM

I've reverted in b1ac1f00716 until it can be fixed properly.

We noticed this in Chromium where we started seeing build spam like:

'+soft-float-abi' is not a recognized feature for this target (ignoring feature)
'+soft-float-abi' is not a recognized feature for this target (ignoring feature)

See https://bugs.chromium.org/p/chromium/issues/detail?id=1023280 for details.

Sorry for not reporting it earlier, but because it didn't break the build per se, we didn't notice until now.

The follow-up patch, implementing this correctly, is D70116.