address @lenary's comment, good catch! thanks a lot!!

address @asb's comment. thanks.

Hi @lenary
This is normal behavior in current clang implementation,
-mcpu=? flag does not interact with any flags, it just print out the all of ProcessorModel registered in backend.
https://github.com/llvm/llvm-project/blob/master/clang/tools/driver/cc1_main.cpp#L213-L215

@asb @lenary I thought this path is ready to land?

avoid to check compiler version in testcase

fix typo

addess @asb 's comment.

BTW, this patch depends on D77030, which aim to avoid the forcing of any ProcessorModel to have FeatureRVCHints feature.
But if we decide to keep the FeatureRVCHints, I need to change implementation a little.

update testcases

address kito's comment

s/ld/lld/

address @MaskRay's review feedback

rebase on master

In D71124#2012090, @kito-cheng wrote:

Another proposal for -mcpu and -mtune:

Decoupling the -mcpu and -mtune option, -mcpu only accept concrete CPU, and -mtune for micro-arch/pipeline model, they accept different option set.

e.g.
-mcpu=sifive-e24 # Imply -march=rv32imafc -mtune=sifive-2-series
-mtune=sifive-2-series # no effect on arch
-mtune=rocket # no effect on arch

So -mcpu=rocket is invalid, since it's micro-arch/pipeline model only,
and -mtune=sifive-e24 is invalid too, because it's CPU not a micro-arch.

In D71124#2010759, @lenary wrote:

This is looking good.

I remember we discussed this on the LLVM call a few weeks ago - there was a discussion as to whether we should be prioritising -march or -mcpu - do you recall the outcome of that discussion?

address @lenary's review feedback

@MaskRay Thanks for your suggestion and reminder.

1. Rebase on master branch.
2. update lld testcases

rebase on D78988

1. Add empty combined module checking before call backend()
2. Appling new approach, some testcasess need to updated and deleted. In checking module empty approach, it can suppress emitting optimzed empty combined module, but in new approach, it still emit combined module. I thought new approach behavior is acceptable. This case happend in linker-script-symbols-assign.ll.
3. Delete llvm/test/ThinLTO/X86/empty-object.ll. I misunderstood -thinlto-distributed-indexes option meaning. I add this case just becasue becuase I modified llvm-lto2. I think testing in empty-module.ll is enough. This patch focus on empty combined module, I think we don't need to comfirm "empty ThinLTO module" still be emitted.

rebase master

address @tejohnson's comments

Hi @tejohnson
I'm very appreciate your help and comment,
because I'm not LTO expert, so if you have any good idea and concern in implementation
Please don't hesitate to correct me, thanks.

fixed linker-script-symbols-assign.ll, now lto would not generate empty module (%t2.lto.o)
fixed typo and format.

sorry, my fault, there are two failing tests I need to fix first.

I plan to suppress to emit empty module or avoid ld to link empty module's object file.

Could you add a testcase? It will helpful to see what issue fixed in this patch. thanks.

Hi @lenary, I added a PoC patch D78035 to complete ThinLTO based on this patch.

I agree negative features is not intuitive, but HINT instruction default is legal and supported, in other words, it mean the disabling HINT instruction is a feature, right?
There were also negative features in other backend. (grep FeatureNo) so we are not first support it.
For sure, I won’t insist on doing this, I feel it's just a friendly trade-off between user and developer.
Thanks. :P

In D77030#1971795, @asb wrote:

I can see the argument for changing the default of EnableRVCHintInstrs. Might we be better just doing that, and keeping it as "rvc-hints" to avoid adding negative features?

@lenary (Sorry for the very late reply...)

update implementation and handle -mcpu with explicitly specified -march.
If -mcpu has default march, explicitly -march will overwrite it.

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 fixed in https://reviews.llvm.org/D70837.

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.
It's not easy to fix it because target-features is per-function basis but ABI is not.