This is an archive of the discontinued LLVM Phabricator instance.

pass -mabi to LTO linker only in RISC-V targets, enable RISC-V LTO
Needs ReviewPublic

Authored by khchen on Dec 11 2019, 8:41 PM.

Details

Summary

ABI(-mabi) is not encoded in bitcode function attribute and it is not suitable to modeled on a per-function basis because one compilation unit need to have the same ABI in risc-v .

in this patch driver will pass -mabi to LTO via option, but there is another passing way via module metadata,
I'm not sure which solution is better.

any suggestion?

Diff Detail

Event Timeline

khchen created this revision.Dec 11 2019, 8:41 PM
khchen updated this revision to Diff 233508.Dec 11 2019, 11:08 PM

update missed code..

Jim added a subscriber: Jim.Dec 12 2019, 12:59 AM
lenary accepted this revision.Dec 16 2019, 5:35 AM

Nice! I think this is the correct way of implementing this, and I don't think it will have any issue with other backends.

This revision is now accepted and ready to land.Dec 16 2019, 5:35 AM

My primary concern with this is that I'm not sure you should be passing this information separately, as opposed to encoding it into the bitcode.

On ARM, the ABI for a file is generally derived from the target triple. For example, "arm-eabi" is soft-float, "arm-eabihf" is hard-float. This makes the intended target clear, provides some protection against accidentally using LTO with files compiled for different ABIs, and matches up well with our existing configuration flags to set a default target for a compiler.

For other ABI-ish sorts of questions, LLVM IR also supports module flags, which can specify various parameters that apply to an entire object file. See, for example, http://llvm.org/docs/LangRef.html#c-type-width-module-flags-metadata . This has some of the same benefits: it clearly applies to the whole file, and it detects mixing targets with LTO.

Jim added a comment.Dec 16 2019, 9:08 PM

This patch is only for RISCV.
The title should have prefix "[RISCV]".

clang/lib/Driver/ToolChains/Arch/RISCV.cpp
729

Argument list should be aligned.

clang/lib/Driver/ToolChains/Arch/RISCV.h
31

Argument list should be aligned.

My primary concern with this is that I'm not sure you should be passing this information separately, as opposed to encoding it into the bitcode.

On ARM, the ABI for a file is generally derived from the target triple. For example, "arm-eabi" is soft-float, "arm-eabihf" is hard-float. This makes the intended target clear, provides some protection against accidentally using LTO with files compiled for different ABIs, and matches up well with our existing configuration flags to set a default target for a compiler.

For other ABI-ish sorts of questions, LLVM IR also supports module flags, which can specify various parameters that apply to an entire object file. See, for example, http://llvm.org/docs/LangRef.html#c-type-width-module-flags-metadata . This has some of the same benefits: it clearly applies to the whole file, and it detects mixing targets with LTO.

@efriedma
Thank you for your information.
Unfortunately on RISCV, the ABI info is only derived from -mabi option and the target triple does not encode ABI info (same to gcc).
In addition, the resulting object file only uses one ABI.

If we choose the module flags metadata solution, there are some problems about which ABI should be chosen when linking different ABIs.

example 1:
clang -target riscv64-unknown-elf a.c -flto -march=rv64imf -mabi=lp64f -o a.o
clang -target riscv64-unknown-elf b.c -flto -march=rv64i -mabi=lp64 -o b.o
clang -target riscv64-unknown-elf -march=rv64imf -mabi=lp64f a.o b.o -flto -o foo

What's expected behavior? user specifics -mabi=lp64f so the result object ABI is lp64f?

example 2:

clang -target riscv64-unknown-elf a.c -flto -march=rv64imf -mabi=lp64f -o a.o
clang -target riscv64-unknown-elf b.c -flto -march=rv64i -mabi=lp64f -o b.o
clang -target riscv64-unknown-elf a.o b.o -flto -o foo

What's expected behavior? The result object is -mabi=lp64f because they have same ABI in module metadata?

example 3:

clang -target riscv64-unknown-elf a.c -flto -march=rv64imf -mabi=lp64f -o a.o
clang -target riscv64-unknown-elf b.c -flto -march=rv64imf -mabi=lp64 -o b.o
clang -target riscv64-unknown-elf a.o b.o -flto -o foo

What's expected behavior? Use the default march and mabi?

RISCV clang driver has default march and mabi, so I think maybe it's a easier and clear way to overwrite ABI via option only for RISCV target,
and there is no problem (I think) when overwriting the original ABI. (so the mixing ABI is not a problem)

BUT in https://reviews.llvm.org/D67385#1680959 @tejohnson taught me that "LLVM are going more and more toward a model where info is communicated via the IR from the compile step",
So I'm wondering if passing the ABI via option to LTO ONLY for specific target, is it still a bad ideal?

Thanks.

khchen marked an inline comment as done.Dec 17 2019, 9:06 AM
khchen added inline comments.
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
729

I will fixed it if when this patch is acceptable, thanks.

Unfortunately on RISCV, the ABI info is only derived from -mabi option and the target triple does not encode ABI info (same to gcc).

How does gcc decide the default ABI for a target? Do you explicitly specify it at configure time?

example 1:
What's expected behavior? user specifics -mabi=lp64f so the result object ABI is lp64f?

I think the expected behavior is a fatal error. There isn't any reason to let people shoot themselves in the foot like this. (module flags have builtin support for producing such an error.)

example 2:
What's expected behavior? The result object is -mabi=lp64f because they have same ABI in module metadata?

Yes.

there is no problem (I think) when overwriting the original ABI. (so the mixing ABI is not a problem)

clang emits different IR for hardfloat vs. softfloat targets; if you try to overwrite the ABI of a bitcode file, we'll generate wrong code.

Unfortunately on RISCV, the ABI info is only derived from -mabi option and the target triple does not encode ABI info (same to gcc).

How does gcc decide the default ABI for a target? Do you explicitly specify it at configure time?

Yes, default March/ABI are decided in configure time, and there is also default March/ABI there.

example 1:
What's expected behavior? user specifics -mabi=lp64f so the result object ABI is lp64f?

I think the expected behavior is a fatal error. There isn't any reason to let people shoot themselves in the foot like this. (module flags have builtin support for producing such an error.)

example 2:
What's expected behavior? The result object is -mabi=lp64f because they have same ABI in module metadata?

Yes.

there is no problem (I think) when overwriting the original ABI. (so the mixing ABI is not a problem)

clang emits different IR for hardfloat vs. softfloat targets; if you try to overwrite the ABI of a bitcode file, we'll generate wrong code.

Do you mean different function attributes for different ABI targets?
I see there are arm_aapcs_vfpcc annotation and use-soft-float=false in attributes when giving the -mfloat-abi=hard option.
But in RISCV clang emits the same IR for different ABI (-mabi), so maybe it is no problem in RISCV? (-mfloat-abi is unused in riscv clang driver)

khchen planned changes to this revision.EditedDec 17 2019, 11:07 PM
khchen added a subscriber: eli.friedman.

I asked @kito-cheng about the GCC LTO behavior,
GCC LTO encodes the ABI info in elf and the behavior in above examples match to @efriedma 's responses.
So I think maybe encode ABI in module metadata flag is a good ideal.

But in RISCV clang emits the same IR for different ABI (-mabi),

This is not true. For simple cases, it does, yes, but there are some weird edge cases for functions with many arguments.

But in RISCV clang emits the same IR for different ABI (-mabi),

This is not true. For simple cases, it does, yes, but there are some weird edge cases for functions with many arguments.

It's interesting, I didn't find the edge cases for functions with many arguments.
but you are right, I found a case when passing struct as argument,
different -mabi will generate incompatible function interface.

struct bar{
int a;
float b;
};

float foo(struct bar a, struct bar b){ ...}

compiled with -march=rv32imafc -mabi=ilp32f: define dso_local float @foo(i32 %0, float %1, i32 %2, float %3)
compiled with -march=rv32imafc -mabi=-ilp32: define dso_local float @foo([2 x i32] %a.coerce, [2 x i32] %b.coerce)

@efriedma thanks for your suggestions.

khchen added a comment.Jan 6 2020, 1:16 AM

I'd like to discuss what is good way to pass target-abi in maillist after I had a try to encoding ABI info into LLVM IR via module flag.
any suggestions are welcome, thanks.

@efriedma To keep you in the loop, D72624 is the patch to add a target hook for setting the TargetMachine options based on module metadata. That patch does not add any RISC-V specific functionality, it only lays the groundwork for it.

Okay. Please let me know if you want me to review anything.

lenary resigned from this revision.Jan 14 2021, 11:33 AM
rkruppe removed a subscriber: rkruppe.Jan 15 2021, 12:56 AM
khchen updated this revision to Diff 345712.May 16 2021, 9:50 AM

Pass -target-abi option into LTO codegenerator base on D102582 patch.

please see D102582 for more detal.

jrtc27 added inline comments.May 16 2021, 9:56 AM
clang/test/Driver/lto.c
81–90

This isn't a great test, anything that unconditionally passes -plugin-opt=target-abi=lp64f passes it. Test RV32 vs RV64 and with multiple ABIs for each to have confidence it does the right thing. Also have a test for not passing anything.

Okay. Please let me know if you want me to review anything.

Hi all,
We had encoded the target-abi into module now, but I feel it does not make sense to
support overwrite ABI option and datalayout in TargetMahcine/IR by target-abi module flag in IR.

So I think maybe passing the target-abi option by clang driver can make anything more simple, the only one limitation is users need to specific -mabi in below cases at the last command.

clang -target riscv64-unknown-elf a.c -flto -march=rv64gc -mabi=lp64f -o a.o
clang -target riscv64-unknown-elf b.c -flto -march=rv64gc -mabi=lp64f -o b.o
clang -target riscv64-unknown-elf a.o b.o -flto -march=rv64gc -o foo

Okay. Please let me know if you want me to review anything.

Hi all,
We had encoded the target-abi into module now, but I feel it does not make sense to
support overwrite ABI option and datalayout in TargetMahcine/IR by target-abi module flag in IR.

So I think maybe passing the target-abi option by clang driver can make anything more simple, the only one limitation is users need to specific -mabi in below cases at the last command.

clang -target riscv64-unknown-elf a.c -flto -march=rv64gc -mabi=lp64f -o a.o
clang -target riscv64-unknown-elf b.c -flto -march=rv64gc -mabi=lp64f -o b.o
clang -target riscv64-unknown-elf a.o b.o -flto -march=rv64gc -o foo

We should treat a missing -mabi= as an implicit -mabi=whatever-the-default-is for consistency with non-LTO. So yes, if the default ABI differs from what you've compiled the .o's with, you should have to provide it. This is needed already _anyway_ for multilib toolchains to determine the right library search path, though there are cases currently when you can get away without providing it, at least with Clang.

khchen added a comment.EditedMay 16 2021, 7:43 PM

Okay. Please let me know if you want me to review anything.

Hi all,
We had encoded the target-abi into module now, but I feel it does not make sense to
support overwrite ABI option and datalayout in TargetMahcine/IR by target-abi module flag in IR.

So I think maybe passing the target-abi option by clang driver can make anything more simple, the only one limitation is users need to specific -mabi in below cases at the last command.

clang -target riscv64-unknown-elf a.c -flto -march=rv64gc -mabi=lp64f -o a.o
clang -target riscv64-unknown-elf b.c -flto -march=rv64gc -mabi=lp64f -o b.o
clang -target riscv64-unknown-elf a.o b.o -flto -march=rv64gc -o foo

We should treat a missing -mabi= as an implicit -mabi=whatever-the-default-is for consistency with non-LTO. So yes, if the default ABI differs from what you've compiled the .o's with, you should have to provide it. This is needed already _anyway_ for multilib toolchains to determine the right library search path, though there are cases currently when you can get away without providing it, at least with Clang.

Hi @jrtc27, do you mean in clang, we need encode an explicitly -target-abi string (computed by RISCVABI::computeTargetABI) rather than an empty string in IR module?

noah95 added a subscriber: noah95.Jun 23 2021, 2:56 AM
khchen updated this revision to Diff 356425.Jul 4 2021, 8:40 PM

Update test cases.

jrtc27 added inline comments.Jul 6 2021, 4:23 PM
clang/lib/Driver/ToolChains/CommonArgs.cpp
633
634–638

ToolChain.getArch().isRISCV()? No need for a switch until there are other architectures that need code here too.

clang/test/Driver/lto.c
126

Give these meaningful names

134–137

Why this set without ilp32 and lp64d, two of the four most common ABIs?

I believe this patch is still relevant/necessary when using LTO for RISCV, so can I ask if @khchen is able to update it to rebase/address the feedback? If not, are there are any objections to me commandeering this revision to get it landed?

I believe this patch is still relevant/necessary when using LTO for RISCV, so can I ask if @khchen is able to update it to rebase/address the feedback? If not, are there are any objections to me commandeering this revision to get it landed?

yes indeed, its still needed. I have disabled LTO for RISCV in Yocto for this reason. I will be happy to test out a rebased patch along.

I believe this patch is still relevant/necessary when using LTO for RISCV, so can I ask if @khchen is able to update it to rebase/address the feedback? If not, are there are any objections to me commandeering this revision to get it landed?

Yes, you could command this revision to enable LTO, thank you!!