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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits