[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

260

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

369

Attrstring -> AttrString

384

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

391

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

385

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

385

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

385

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

6

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

417

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

376

- 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

[BeMg]

ping

[BeMg]

Align with spec

1. duplicate target attribute will cause compilation fail

[craig.topper]

clang/lib/Basic/Targets/RISCV.cpp

229

Can we use something like

auto I = llvm::find(FeaturesVec, "__RISCV_TargetAttrNeedOverride");
if (I == FeaturesVec.end())
  return FeaturesVec;

return std::vector(I++, FeaturesVec.end());

240

Does this lose features like relax and save-restore? Those aren't part of the -march so they don't appear after __RISCV_TargetAttrNeedOverride

411

You don't need an else since the body of the if ended in continue. When the if is taken control flow will never reach here.

421

Why does this clear Ret.Features, but full-arch-string doesn't?

427

No need for else here

[BeMg]

clang/lib/Basic/Targets/RISCV.cpp

421

I think full-arch-string also clear the Ret.Features in line 398 right after if (Feature.startswith("arch=")).

[craig.topper]

clang/lib/Basic/Targets/RISCV.cpp

357

Why do we need "__RISCV_TargetAttrNeedOverride"?

421

Thanks. I missed it.

[BeMg]

1. remain else if by removing continue
2. Keep NonISAExtFeature after override
3. Update testcase with -target-feature +save-restore
4. Improve resolveTargetOverride function

[BeMg]

clang/lib/Basic/Targets/RISCV.cpp

357

My idea is using __RISCV_TargetAttrNeedOverride to distinguish the module-level feature and target attribute feature.

The module-level feature will merge with target attribute feature in https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ASTContext.cpp#L13481. And be used/refine inside RISCVTargetInfo::initFeatureMap.

After merge, It could find the target attribute feature start point by analyzing FeatureVec, but I prefer use the __RISCV_TargetAttrNeedOverride to do this job.

[BeMg]

ping

[craig.topper]

clang/lib/Basic/Targets/RISCV.cpp

229

Do we need to call find again? We already did it in the caller, can we pass that information somehow?

230

std::vector<std::string> FeaturesNeedOverride(FeaturesVec.begin(), I);

243

llvm::copy_if(FeatureNeedOveride, std::back_inserter(NonISAExtFeatureVec), [&](const std::string &Feat) { return !llvm::is_contained(ImpliedFeatures, Feat); });

[BeMg]

1. reuse the find result
2. replace for loop with llvm::copy_if
3. use explicit std::vector declaration instead of auto

[craig.topper]

LGTM

[sylvestre.ledru]

looks like it caused:
https://github.com/llvm/llvm-project/issues/73303

[reames]

In local testing, I'm seeing some very odd behavior with target attributes. It looks like target attributes are being properly handled if clang -cc1 is directly invoked, but not if the clang driver is used. Or at least that's currently my best theory.

This works:
/home/preames/llvm-dev/build/bin/clang -cc1 -triple riscv64 ~/rivos-llvm/llvm-project/clang/test/CodeGen/RISCV/riscv-func-attr-target.c -S -emit-llvm -o - -O3

This does not:
/home/preames/llvm-dev/build/bin/clang -target riscv64-unknown-linux-gnu ~/rivos-llvm/llvm-project/clang/test/CodeGen/RISCV/riscv-func-attr-target.c -S -emit-llvm -o - -O3

[reames]

I think I figured out what is going on here.

The clang driver materializes all of the target-features at the cc1 invocation command line (including the negative ones). If I manually duplicate this at the cc1 command line, that one stops working.

clang/lib/Basic/Targets/RISCV.h

public:
   bool useFP16ConversionIntrinsics() const override {
     return false;
   }
 
   bool isValidCPUName(StringRef Name) const override;
   void fillValidCPUList(SmallVectorImpl<StringRef> &Values) const override;
   bool isValidTuneCPUName(StringRef Name) const override;
   void fillValidTuneCPUList(SmallVectorImpl<StringRef> &Values) const override;
+  bool supportsTargetAttributeTune() const override { return true; }
+  bool validateCpuSupports(StringRef FeatureStr) const override;
+  ParsedTargetAttr parseTargetAttr(StringRef Str) const override;
 };
 class LLVM_LIBRARY_VISIBILITY RISCV32TargetInfo : public RISCVTargetInfo {
 public:
   RISCV32TargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
       : RISCVTargetInfo(Triple, Opts) {
     IntPtrType = SignedInt;
     PtrDiffType = SignedInt;
     SizeType = UnsignedInt;

clang/lib/Basic/Targets/RISCV.cpp

 
 ArrayRef<Builtin::Info> RISCVTargetInfo::getTargetBuiltins() const {
   return llvm::ArrayRef(BuiltinInfo,
                         clang::RISCV::LastTSBuiltin - Builtin::FirstTSBuiltin);
 }
 
 bool RISCVTargetInfo::initFeatureMap(
     llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags, StringRef CPU,
     const std::vector<std::string> &FeaturesVec) const {
 
   unsigned XLen = 32;
 
   if (getTriple().getArch() == llvm::Triple::riscv64) {
     Features["64bit"] = true;
     XLen = 64;
   } else {
     Features["32bit"] = true;
   }
 
   auto ParseResult = llvm::RISCVISAInfo::parseFeatures(XLen, FeaturesVec);
   if (!ParseResult) {
     std::string Buffer;
     llvm::raw_string_ostream OutputErrMsg(Buffer);
     handleAllErrors(ParseResult.takeError(), [&](llvm::StringError &ErrMsg) {
       OutputErrMsg << ErrMsg.getMessage();
     });
     Diags.Report(diag::err_invalid_feature_combination) << OutputErrMsg.str();
     return false;
   }
 
   // RISCVISAInfo makes implications for ISA features
   std::vector<std::string> ImpliedFeatures = (*ParseResult)->toFeatureVector();
+  std::vector<std::string> UpdatedFeatures;
+
   // Add non-ISA features like `relax` and `save-restore` back
   for (const std::string &Feature : FeaturesVec)
     if (!llvm::is_contained(ImpliedFeatures, Feature))
-      ImpliedFeatures.push_back(Feature);
+      UpdatedFeatures.push_back(Feature);
+
+  UpdatedFeatures.insert(UpdatedFeatures.end(), ImpliedFeatures.begin(),
+                         ImpliedFeatures.end());
 
-  return TargetInfo::initFeatureMap(Features, Diags, CPU, ImpliedFeatures);
+  return TargetInfo::initFeatureMap(Features, Diags, CPU, UpdatedFeatures);
 }
 
 std::optional<std::pair<unsigned, unsigned>>
 RISCVTargetInfo::getVScaleRange(const LangOptions &LangOpts) const {
   // RISCV::RVVBitsPerBlock is 64.
   unsigned VScaleMin = ISAInfo->getMinVLen() / llvm::RISCV::RVVBitsPerBlock;
 
   if (LangOpts.VScaleMin || LangOpts.VScaleMax) {
bool RISCVTargetInfo::isValidTuneCPUName(StringRef Name) const {
   return llvm::RISCV::parseTuneCPU(Name, Is64Bit);
 }
 
 void RISCVTargetInfo::fillValidTuneCPUList(
     SmallVectorImpl<StringRef> &Values) const {
   bool Is64Bit = getTriple().isArch64Bit();
   llvm::RISCV::fillValidTuneCPUArchList(Values, Is64Bit);
 }
+
+// Parse RISC-V Target attributes, which are a comma separated list of:
+//  "arch=<arch>" - parsed to features as per -march=..
+//  "cpu=<cpu>" - parsed to features as per -mcpu=.., with CPU set to <cpu>
+//  "tune=<cpu>" - TuneCPU set to <cpu>
+ParsedTargetAttr RISCVTargetInfo::parseTargetAttr(StringRef Features) const {
+  ParsedTargetAttr Ret;
+  if (Features == "default")
+    return Ret;
+  SmallVector<StringRef, 1> AttrFeatures;
+  Features.split(AttrFeatures, ";");
+  bool FoundArch = false;
+
+  for (auto &Feature : AttrFeatures) {
+    Feature = Feature.trim();
+    StringRef AttrString = Feature.split("=").second.trim();
+
+    if (Feature.startswith("arch=")) {
+      if (FoundArch)
+        Ret.Duplicate = "arch=";
+      FoundArch = true;
+
+      if (AttrString.startswith("+") || AttrString.startswith("-")) {
+        // EXTENSION like arch=+v,+zbb,-c
+        SmallVector<StringRef, 1> Exts;
+        AttrString.split(Exts, ",");
+        for (auto Ext : Exts) {
+          if (Ext.empty())
+            continue;
+
+          StringRef ExtName = Ext.substr(1);
+          std::string TargetFeature =
+              llvm::RISCVISAInfo::getTargetFeatureForExtension(ExtName);
+          if (!TargetFeature.empty())
+            Ret.Features.push_back(Ext.front() + TargetFeature);
+          else
+            Ret.Features.push_back(Ext.str());
+        }
+      } else {
+        // full-arch-string like arch=rv64gcv
+        auto RII = llvm::RISCVISAInfo::parseArchString(
+            AttrString, /* EnableExperimentalExtension */ true);
+        if (!RII) {
+          consumeError(RII.takeError());
+        } else {
+          std::vector<std::string> FeatStrings = (*RII)->toFeatureVector();
+          for (auto FeatString : FeatStrings)
+            Ret.Features.push_back(FeatString);
+        }
+      }
+      continue;
+    } else if (Feature.startswith("cpu=")) {
+      if (!Ret.CPU.empty())
+        Ret.Duplicate = "cpu=";
+      else
+        Ret.CPU = AttrString;
+      continue;
+    } else if (Feature.startswith("tune=")) {
+      if (!Ret.Tune.empty())
+        Ret.Duplicate = "tune=";
+      else
+        Ret.Tune = AttrString;
+      continue;
+    } else if (Feature.startswith("no-"))
+      Ret.Features.push_back("-" + Feature.split("-").second.str());
+    else
+      Ret.Features.push_back("+" + Feature.str());
+  }
+  return Ret;
+}
+
+bool RISCVTargetInfo::validateCpuSupports(StringRef FeatureStr) const {
+  return ISAInfo->isSupportedExtensionFeature(FeatureStr);
+}

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

+// REQUIRES: riscv-registered-target
+// RUN: %clang_cc1 -triple riscv64 -target-feature +zifencei \
+// RUN:  -target-feature +m -target-feature +a               \
+// RUN:  -target-feature +f -target-feature +d               \
+// RUN:  -emit-llvm %s -o - | FileCheck %s                   \
+// RUN:  --check-prefix=CHECK-IR
+
+// CHECK-IR: void @test1() #0
+__attribute__((target("arch=-a,+v,+zbb,+zihintntl0p2"))) void test1() {}
+
+// CHECK-IR: void @test2() #1
+__attribute__((target("arch=rv64gc_zbb"))) void test2 () {}
+
+// CHECK-IR: void @test3() #2
+__attribute__((target("cpu=rocket-rv64;tune=generic-rv64;arch=+v"))) void test3 () {}
+
+// CHECK-IR: attributes #0 {{.*}}+experimental-zihintntl{{.*}}+v,+zbb{{.*}}+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b,-a{{.*}}
+// CHECK-IR: attributes #1 {{.*}}+c{{.*}}+zbb{{.*}}
+// CHECK-IR: attributes #2 {{.*}} "target-cpu"="rocket-rv64" {{.*}}+v{{.*}} "tune-cpu"="generic-rv64" {{.*}}
 No newline at end of file

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

+; RUN: llc -mtriple=riscv64 -mattr=+a,+d,+f,+m -verify-machineinstrs < %s | FileCheck %s
+
+; CHECK: .option push
+; CHECK-NEXT: .option arch, +c, +v, +zifencei, +zve32f, +zve32x, +zve64d, +zve64f, +zve64x, +zvl128b, +zvl32b, +zvl64b
+define void @test1() "target-features"="+a,+d,+f,+m,+c,+v,+zifencei,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b" {
+; CHECK-LABEL: test1
+; CHECK: .option pop
+entry:
+  ret void
+}
+
+; CHECK: .option push
+; CHECK-NEXT: .option        arch,   +c, +zifencei
+define void @test2() "target-features"="+a,+d,+f,+m,+c,+zifencei" {
+; CHECK-LABEL: test2
+; CHECK: .option pop
+entry:
+  ret void
+}
+
+; CHECK: .option push
+; CHECK-NEXT: .option arch, +v, +zifencei, +zve32f, +zve32x, +zve64d, +zve64f, +zve64x, +zvl128b, +zvl32b, +zvl64b
+define void @test3() "target-features"="+a,+d,+f,+m,+v,+zifencei,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b" {
+; CHECK-LABEL: test3
+; CHECK: .option pop
+entry:
+  ret void
+}
+
+; CHECK: .option push
+; CHECK-NEXT: .option arch, +experimental-zihintntl, +zifencei
+define void @test4() "target-features"="+a,+d,+f,+m,+experimental-zihintntl,+zifencei" {
+; CHECK-LABEL: test4
+; CHECK: .option pop
+entry:
+  ret void
+}
+
+; CHECK: .option push
+; CHECK-NEXT: .option arch, +zifencei
+define void @test5() "target-features"="+a,+d,+f,+m,+zifencei" {
+; CHECK-LABEL: test5
+; CHECK: .option pop
+entry:
+  ret void
+}