[RISCV] Support target attribute for function

[BeMg]

Summary

The proposal of target attribute is https://github.com/riscv-non-isa/riscv-c-api-doc/pull/35

This patch implements it by emitting .option arch during codegen.

[craig.topper]

clang/test/CodeGen/RISCV/riscv-func-attr-target.c

2

No test for tune=?

[BeMg]

update

[kito-cheng]

Testcase for backend?

[BeMg]

1. Update RTS's function name
2. Add backend testcase

[kito-cheng]

clang/test/CodeGen/RISCV/riscv-func-attr-target.c

11

It's ext list or full arch, can't mixed together.

ARCH-ATTR   := 'arch=' EXTENSIONS-OR-FULLARCH
EXTENSIONS-OR-FULLARCH := <EXTENSIONS>
                        | <FULLARCHSTR>

35

The extension name is zihintntl not experimental-zihintntl, I would suggest either drop the experimental extension support or requiring the version number to be specified.

[BeMg]

1. Update testcase
2. Support -<EXTENSION>
3. Verfy extension version

[BeMg]

Use isSupportedExtensionWithVersion to implement parseTargetAttr

[MaskRay]

clang/test/CodeGen/RISCV/riscv-func-attr-target.c

3

-target has been deprecated since Clang 3.x. Use --target= for new tests.

Prefer %clang_cc1 for non-Driver tests. Only use %clang in test/Driver

6

We don't format tests, so this annotation is unneeded.

[craig.topper]

clang/include/clang/Basic/TargetInfo.h

1401 ↗

(On Diff #529510)

Is this needed for target attribute? The description to me reads like its for doing runtime dispatch with ifunc.

clang/lib/Basic/Targets/RISCV.cpp

276

UpdatedFeatures.insert(UpdatedFeatures.end(), ImpliedFeatures.begin(), ImpliedFeatures.end())

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

247 ↗

(On Diff #529510)

const auto &Feature

[BeMg]

1. Update testcase
2. Remove supportsMultiVersioning for RISCV

[craig.topper]

clang/lib/Basic/Targets/RISCV.cpp

382

Attrstring -> AttrString

397

Why std::string, don't all 3 functions its passed to take a StringRef?

404

We can't write directly to llvm::errs(). It needs to go through clang's Diagnostics code.

[BeMg]

1. Attrstring -> AttrString
2. using StringRef instead of string
3. Append unsupported target feature Let follow-up procedure raise diagnostic code

[BeMg]

Add const

[craig.topper]

clang/lib/Basic/Targets/RISCV.cpp

398

I wonder if we could encapsulate this if and the 3 calls into RISCVISAInfo into a single function in RISCVISAInfo?

Basically we want to know if an extension that may or may not have a version, is a valid extension and if so what is the target feature name for it. We could have one function that does all that and returns the target feature name or an empty string.

[craig.topper]

clang/lib/Basic/Targets/RISCV.cpp

398

I think I explained my idea poorly. I was wondering about having a function that allowed us to write

std::string TargetFeature = llvm::RISCVISAInfo::getTargetFeatureForExtension(ExtName);
if (!TargetFeature.empty())
  Ret.Features.push_back(Ext.front() + TargetFeature);
else
  Ret.Features.push_back(Ext.str());

[BeMg]

clang/lib/Basic/Targets/RISCV.cpp

398

Thank you for further explanation. I think getTargetFeatureFromOneExt can do this job.

std::string TargetFeature = llvm::RISCVISAInfo::getTargetFeatureFromOneExt(ExtName);
if (!TargetFeature.empty())
    Ret.Features.push_back(Ext.front() + TargetFeature);
else
    Ret.Features.push_back(Ext.str());

[BeMg]

Use getTargetFeatureForExtension instead of isSupportExtension

[reames]

Can you separate the change to RISCVAsmPrinter.cpp into it's own review? This looks useful for any case where we have functions in the same model with different function attributes. The attribute((target...) syntax is one way to have that, but there are also others. LTO will see this, and so may other frontends.

This will also help with review as the set of people who can review RISCV backend changes and clang frontend changes is fairly non-overlapping.

llvm/test/CodeGen/RISCV/riscv-func-attr-target.ll

5 ↗

(On Diff #535653)

This test would be a lot easier to follow if you pruned all the spurious attributes, and uses the inline attribute syntax rather than the #0 reference syntax.

[reames]

FYI, this change appears to be a partial diff. Locally, I need to add RISCV to supportsMultiVersioning() to get this to work at all.

[jrtc27]

Isn't multiversioning a separate thing that builds on top of per-function target attributes?

[craig.topper]

In D151730#4491773, @jrtc27 wrote:

Isn't multiversioning a separate thing that builds on top of per-function target attributes?

That's what I thought. The supportsMultiVersioning call was in an earlier version of the patch that I asked about.

[BeMg]

1. simplify riscv-func-attr-target.ll
2. handle corner case in parseTargetAttr

[reames]

In D151730#4491786, @craig.topper wrote:

In D151730#4491773, @jrtc27 wrote:

Isn't multiversioning a separate thing that builds on top of per-function target attributes?

That's what I thought. The supportsMultiVersioning call was in an earlier version of the patch that I asked about.

Yeah, looks like I got confused.

I hadn't expected that target("default") would automatically be considered multi-versioning, even when there was one definition of the function. As such, my test case was accidentally testing multi-versioning even when I hadn't meant to.

[BeMg]

separate the change to RISCVAsmPrinter.cpp into another patch

[jrtc27]

• Asm tests are a no in Clang
• You seem to have some crazy long lists that I doubt need to be that long for test coverage (nor do you need so many variations, surely)
• CHECL is not CHECK
• You have some very strange spacing in CHECK lines

[BeMg]

Update testcase

[BeMg]

Move riscv-func-attr-target.ll into D155155

[BeMg]

rebase and update testcase

[craig.topper]

clang/lib/Basic/Targets/RISCV.cpp

430

Is this tested? I don't see any "no-" in the the test

[BeMg]

Remove some condition because RFC doesn't support it. (Must include prefix arch|cpu|tune)

[wangpc]

clang/lib/Basic/Targets/RISCV.cpp

389

- is removed in https://github.com/riscv-non-isa/riscv-c-api-doc/pull/35/commits/fc690d6caf71c80500d2572dd04d87773946451a.

[BeMg]

Also remove the - operator from target attribute

[BeMg]

Update to lastest spec

1. When it exist the duplicate target attribute, select the lastest one.
2. arch's features will override cpu's features. TargetFeature will override commandline feature.
3. enhence the testcase