Page MenuHomePhabricator

[RISCV] add subtargets initialized with target feature
AcceptedPublic

Authored by khchen on Tue, Nov 12, 2:56 AM.

Details

Summary

According to https://reviews.llvm.org/D67409#1737444, previous enabling LTO patch uses a wrong approach.
The target features have been encoded in bitcode files on a per-function basis, so clang driver does not need to pass them to LTO code generator.
So I reference ARM backend to init subtarget if there is target features attribute.

Diff Detail

Event Timeline

khchen created this revision.Tue, Nov 12, 2:56 AM
lenary added a subscriber: efriedma.

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'm going to add @efriedma to the reviewers, as he flagged up this was implemented in the wrong way before.

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.

I'm going to add @efriedma to the reviewers, as he flagged up this was implemented in the wrong way before.

khchen planned changes to this revision.EditedThu, Nov 14, 5:52 AM

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.

In addition, I think my testcase is so weird and it does not make sense there are different isa extension are used in the same compilation unit...
maybe before checking the invalid ABI, backend need to gather all isa-extension target-feature attribute from functions and decide which standard extension should used if user does not specific -march.
or assume in RISC-V "isa extension target feature" should be specific in codegen and the clang driver treat them as specific case so driver will only pass "isa extension target feature" into LTO code generator.

kito-cheng added a comment.EditedThu, Nov 14, 7:25 AM

In addition, I think my testcase is so weird and it does not make sense there are different isa extension are used in the same compilation unit...

It's not weird, there is real use case in glibc work with IFUNC, target attribute for AArch64 and x86 can result different target feature in same compilation unit, and I believe such feature will implement for RISC-V in future.

void foo(){
}

void bar() __attribute__((target("sse4")));
void bar(){
}
khchen requested review of this revision.Tue, Nov 26, 6:02 PM

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.

It looks like assembler need to read the assembly directive like .attribute arch, rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0 to initial the target feature before abi checking (computeTargetABI).
so I will fix this part (generate and parse assembly directive) later.

In addition, I think my testcase is so weird and it does not make sense there are different isa extension are used in the same compilation unit...
maybe before checking the invalid ABI, backend need to gather all isa-extension target-feature attribute from functions and decide which standard extension should used if user does not specific -march.
or assume in RISC-V "isa extension target feature" should be specific in codegen and the clang driver treat them as specific case so driver will only pass "isa extension target feature" into LTO code generator.

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.

khchen added a comment.EditedWed, Nov 27, 4:15 AM

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.

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.

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.

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.

my original plan is assume there is only one ABI in one compilation unit, so just pass -target-abi to LTO backend via option, and fix the above issues I meet.
But if in the future the object file allow different functions has different ABIs in one compilation unit, it should be have per function ABI feature in backend.
I noted there is no one target looks soft-float-abi attribute to decide ABI, they almost uses soft-float.
so if rv32imafc/ilp32 combination (like arm's softfp) is illegal, I think using soft-float is enough and a good ideal.

We definitely want to support rv32imafc/ilp32 and combinations like that.

At some point we're going to have to reckon with the fact that the ABI is described globally in the resulting object file, so cannot really be modelled on a per-function basis.

I think you're right that the solution for the moment is to pass -target-abi down to LTO, but only for RISC-V targets. We can work out what else we need to do for LTO once we're at that point.

khchen updated this revision to Diff 231474.EditedThu, Nov 28, 7:46 PM

add expected failed case, it fixed in https://reviews.llvm.org/D70837.

efriedma accepted this revision.Tue, Dec 3, 4:11 PM

LGTM

llvm/test/CodeGen/RISCV/subtarget-features-std-ext.ll
6

Not really an issue with this patch specifically, but backends shouldn't be writing directly to stderr. There's a diagnose() API on LLVMContext.

This revision is now accepted and ready to land.Tue, Dec 3, 4:11 PM
khchen marked 2 inline comments as done.Tue, Dec 3, 6:15 PM
khchen added inline comments.
llvm/test/CodeGen/RISCV/subtarget-features-std-ext.ll
6

got it, I will fix it, thanks.