[PATCH] D151730: [RISCV] Support target attribute for function

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:229
+resolveTargetAttrOverride(const std::vector<std::string> &FeaturesVec) {
+  if (!llvm::is_contained(FeaturesVec, "__RISCV_TargetAttrNeedOverride"))
+    return FeaturesVec;
----------------
Can we use something like

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

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


================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:255
 
-  auto ParseResult = llvm::RISCVISAInfo::parseFeatures(XLen, FeaturesVec);
+  std::vector<std::string> NewFeaturesVec =
+      resolveTargetAttrOverride(FeaturesVec);
----------------
Does this lose features like `relax` and `save-restore`? Those aren't part of the -march so they don't appear after `__RISCV_TargetAttrNeedOverride`


================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:424
+      continue;
+    } else if (Feature.startswith("cpu=")) {
+      if (!Ret.CPU.empty())
----------------
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.


================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:434
+        if (MarchFromCPU != "") {
+          Ret.Features.clear();
+          handleFullArchString(MarchFromCPU, Ret.Features);
----------------
Why does this clear Ret.Features, but full-arch-string doesn't?


================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:440
+      continue;
+    } else if (Feature.startswith("tune=")) {
+      if (!Ret.Tune.empty())
----------------
No need for `else` here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151730/new/

https://reviews.llvm.org/D151730