I fixed bug here, but I'm not sure this small fixed should be another commit or not.
@lenary, do you have any suggestion?

In D70837#1824682, @RKSimon wrote:

@khchen We have a load of EXPENSIVE_CHECKS RISCV MC + asm failures, a lot of which appear to be related to these changes to RISCVAsmParser please can you take a look?

First appeared:
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/21599/

Current:
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/21630/steps/test-check-all/logs/stdio

In D72755#1821540, @lenary wrote:

Please can you also add code for ensuring that the target-abi module flag matches the -target-abi command line flag in llvm?

In D72624#1818605, @khchen wrote:

I think putting the resetTargetDefaultOptions after instance of TargetMachine is too late.
for example:
ppc and mips compute the TargetABI in XXXXTargetMachine constructor. In addition , mips compute the DataLayout with target ABI in TargetMachine constructor.

Hi @lenary,
This patch is necessary for enabling LTO
backend does this mismatch checking before getSubtargetImpl which reads the target-feature info from IR function attribute, and it will reset the target-abi if mismatching happen.
because I don't want to break this mismatch checking feature, so I move this checking later than getSubtargetImpl execution.

I think putting the resetTargetDefaultOptions after instance of TargetMachine is too late.
for example:
ppc and mips compute the TargetABI in XXXXTargetMachine constructor. In addition , mips compute the DataLayout with target ABI in TargetMachine constructor.

Hi @efriedma
Could you please guide me and review this PoC?
or take a look at this [[ take a look at | maillist ]] thread?
Thank you!

remote LTO related code.
this PoC include D70837 patch for generate correct code.

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.

The problem is how -mcpu interact with explicitly specified -march (or target features).

In D71387#1790413, @efriedma wrote:

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.

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.

In D71387#1788472, @efriedma wrote:

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?

In D71387#1786518, @efriedma wrote:

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.

update missed code..

BTW, I used this patch and D71124 to run the CoreMark on HiFive unleashed with -O3 -mllvm -enable-misched -mllvm -enable-post-misched -mcpu=rocket-rv64,
the result is better than -O3 -mllvm -enable-misched -mllvm -enable-post-misched and get the performance 3.5% speedup (use ld.bfd and libgcc).

update summary.

add expected failed case, it will be fixed in a subsequent patch.

In D70116#1761610, @lenary wrote:

In D70116#1761541, @khchen wrote:

In D70116#1761492, @lenary wrote:

So am I right in thinking that neither foo nor foo2 will trigger the ABI_Unknown issue, and the issue is only coming from hand-written assembly files?

Actually, you may need to specify -target-abi in the top of the subtarget-features-std-ext.ll file.

Sorry I didn't explain clearly,
The problem is caused by when I try to enable LTO and it need to pass mabi to LTO code generator (mabi is not encoded in bitcode)
like we compile below case with llc example.c -mtriple=riscv32 -mabi=ilp32f.

define float @foo(i32 %a) nounwind #0 {
  %conv = sitofp i32 %a to float
  ret float %conv
}

define float @foo2(i32 %a) nounwind #1{
  %conv = sitofp i32 %a to float
  ret float %conv
}

attributes #0 = { "target-features"="+f"}
attributes #1 = { "target-features"="+f"}

and they will show two error messages Hard-float 'f' ABI can't be used for a target that doesn't support the F instruction set extension.
so there is no any issue in subtarget-features-std-ext.ll without -target-abi.

when I started to fix this error, I found backend shares the ABI parsing/checking function (computeTargetABI) in codegen and MC layer when instance RISCVAsmBackend (note: IR function is not available at this point)
ref. https://github.com/llvm/llvm-project/commit/fea4957177315f83746dca90cb4c9013eb465c46

so this patch is workable, but it's not useful for LTO, not useful enough to support per function feature in backend, and then I'm not sure it's worth to upstream this patch.

This patch seems to solve the issue of working out the correct Subtarget on a per-function basis? This would suggest it is useful.

Yes. Okay, agree with you.

Presumably the next steps are to work out how to pass -target-abi through to the LTO backend via attributes? I was trying to see how other backends did this, but I'm not sure. I think the right way may be to have a feature per ABI, like ARM does here: https://github.com/llvm/llvm-project/blob/2d739f98d8a53e38bf9faa88cdb6b0c2a363fb77/clang/lib/Driver/ToolChains/Arch/ARM.cpp#L313-L319

It might be worth sending an email to llvm-dev, asking for guidance.

In D70116#1761492, @lenary wrote:

So am I right in thinking that neither foo nor foo2 will trigger the ABI_Unknown issue, and the issue is only coming from hand-written assembly files?

Actually, you may need to specify -target-abi in the top of the subtarget-features-std-ext.ll file.

In D70116#1745601, @khchen wrote:

I found RISCVABI::computeTargetABI will check ABI with target feature,
so when I apply this patch and enabling lto, the checking get failed and set ABI to ABI_Unknown.
It's not easy to fix it because target-features is per-function basis but ABI is not.

Re-applied with test fix in https://reviews.llvm.org/rG4fccd383d571865321b4723b81c3042d2c15fd80

@lenary failed again,
http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/483/steps/test-check-clang/logs/FAIL%3A%20Clang%3A%3Ariscv32-toolchain.c
http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/483/steps/test-check-clang/logs/FAIL%3A%20Clang%3A%3Ariscv64-toolchain.c
I will setup windows platform to figure out what's going on.

rebase and fix failed testcases

rebase

I found RISCVABI::computeTargetABI will check ABI with target feature,
so when I apply this patch and enabling lto, the checking get failed and set ABI to ABI_Unknown.
This is a problem because target-features is per-function basis but ABI is not.

In D70116#1741987, @lenary wrote:

Nice, if this is the correct way to do it, then it's looking good.

I see x86 and arm both inspect the "soft-float" attribute, which seems to come from the -msoft-float clang option. Should RISC-V be using that to switch off the d and f target features?

I think in RISC-V using -mabi is a convention way for specific calling convention in gcc and clang, and more clear than -msoft-float.
but unfortunately compiling source code with -mabi=ilp32 will not generate correct value like "use-soft-float"="true" in bitcode,
so we can fix clang (?) and let backend inspect the "soft-float" attribute, or fix clang driver to pass -mabi to LTO code generator in RISC-V target.

LGTM, thanks for the patch!

@lenary
You patch is very useful to look up the default march, thanks!
But there is some issue if we set the default rv32 march as rv32gc.
Because the default multilib does not include rv32gc/lp32d in riscv gnu toolchain, https://github.com/riscv/riscv-gcc/blob/ed3f6ec/gcc/config/riscv/t-elf-multilib#L19
so if user does not give the default march clang will not find the library and cause linking error.
Do you think this is a clang's problem? or maybe this is just user's problem so they should config multilib to support rv32gc/lip3d2d?

@lenary Sorry, I don't have the plan to support MULTILIB_REUSE now.
But if it's necessary to support it, I can do it later.

In D67409#1707940, @lenary wrote:

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

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

In D67385#1680959, @tejohnson wrote:

In D67385#1680942, @khchen wrote:

@tejohnson for example:

$ clang -flto a.c -c -o a.o
$ llvm-ar q a.a a.o          // archive libraries      
$ clang -flto a.a b.c -O2 -o main -mcmodel=small 

In above case user need to aware that passing -Wl,-plugin-opt=-code-model=small to plugin is necessary.
It seems to me that is inconvenience because I believe user doesn't know it.

In general, -flto should be transparent to the build process. Think about what happens in your example if you remove -flto:

$ clang a.c -c -o a.o
$ llvm-ar q a.a a.o // archive libraries
$ clang a.a b.c -O2 -o main -mcmodel=small

a.o will be a native code built with the default mcmodel and -O0.

Generally, adding -flto shouldn't change this - I would argue that it would be unexpected to apply mcmodel=small as the user chose to compile a.c with the default mcmodel. We are going more and more toward a model where info is communicated via the IR from the compile step, as the mcmodel already is. Is there a compelling reason why a.c should not get the default mcmodel in this case (without having compiled it mcmodel=small in the first place)?

@tejohnson for example:

$ clang -flto a.c -c -o a.o
$ llvm-ar q a.a a.o          // archive libraries      
$ clang -flto a.a b.c -O2 -o main -mcmodel=small 

@tejohnson when I run the clang -flto -Wl,-plugin-opt=-help command, it shows the

--code-model=<value>                               - Choose code model
  =tiny                                            -   Tiny code model
  =small                                           -   Small code model
  =kernel                                          -   Kernel code model
  =medium                                          -   Medium code model
  =large                                           -   Large code model

so I think the clang driver need to pass this info to LTO codegen, does it make sense?

ping

ping

ping

Thanks @lenary, I fixed it.

added a test

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

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