Page MenuHomePhabricator

pass -mabi to LTO linker only in RISC-V targets, enable RISC-V LTO
Changes PlannedPublic

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
585

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
585

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.