[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

zixuan-wu added a comment. Herald added subscribers: VincentWu, luke957. Hi, @kito-cheng as your this patch unify the extension handing in one same place by new infra, are you going to support the multiple extension version in next step? Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

zixuan-wu added inline comments. Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp:107 + for (auto Feature : RISCVFeatureKV) { +if (FeatureBits[Feature.Value] && +llvm::RISCVISAInfo::isSupportedExtensionFeature(Feature.Key)) If

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

craig.topper added a comment. In D105168#3071330 , @jrtc27 wrote: > In D105168#3071321 , @craig.topper > wrote: > >> In D105168#3071249 , @jrtc27 wrote: >> >>> Two

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

jrtc27 added a comment. In D105168#3071321 , @craig.topper wrote: > In D105168#3071249 , @jrtc27 wrote: > >> Two options come to mind if we really need to be outputting a StringRef list: >> >> 1. (the simpler

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

craig.topper added a comment. In D105168#3071249 , @jrtc27 wrote: > Two options come to mind if we really need to be outputting a StringRef list: > > 1. (the simpler option) Pass in a Twine -> `const char *` lambda that the > caller hooks up to

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

jrtc27 added a comment. Two options come to mind if we really need to be outputting a StringRef: 1. (the simpler option) Pass in a Twine -> `const char *` lambda that the caller hooks up to Args.MakeArgString 2. (probably the nicer option) Invent our own MakeArgString that allocates from

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

teemperor added a comment. In D105168#3071152 , @craig.topper wrote: > In D105168#3069499 , @teemperor > wrote: > >> This broke the modules build: >> >> While building module 'LLVM_Utils' imported from >>

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

craig.topper added a comment. In D105168#3069499 , @teemperor wrote: > This broke the modules build: > > While building module 'LLVM_Utils' imported from > lvm/lib/Support/TargetParser.cpp:14: > In file included from :209: >

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

teemperor added a comment. This broke the modules build: While building module 'LLVM_Utils' imported from lvm/lib/Support/TargetParser.cpp:14: In file included from :209: llvm/include/llvm/Support/RISCVISAInfo.h:15:10: fatal error: could not build module 'LLVM_Option' #include

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

akuegel added inline comments. Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:15 +#include "llvm/ADT/StringRef.h" +#include "llvm/Option/ArgList.h" +#include "llvm/Support/Error.h" Would it be possible to avoid the usage of ArgList in a target that belongs

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng added inline comments. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:39 + +static constexpr StringLiteral AllStdExts = "mafdqlcbjtpvn"; + kito-cheng wrote: > craig.topper wrote: > > 'b' shouldn't be in this list anymore? > I would prefer to keep that

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng added a comment. Committed with one minor update for version of zba/zbb/zbc/zbs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105168/new/ https://reviews.llvm.org/D105168 ___ cfe-commits

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGff13189c5d0d: [RISCV] Unify the arch string parsing logic to to RISCVISAInfo. (authored by Kito Cheng kito.ch...@sifive.com). Changed prior to

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng added inline comments. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:39 + +static constexpr StringLiteral AllStdExts = "mafdqlcbjtpvn"; + craig.topper wrote: > 'b' shouldn't be in this list anymore? I would prefer to keep that for sync with ISA

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

khchen added inline comments. Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2089 else return Error(ValueExprLoc, "bad arch string " + Arch); } maybe we could have a NFC patch later to add ``` // Emit the arch string if needed. if

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

craig.topper accepted this revision. craig.topper added a comment. This revision is now accepted and ready to land. LGTM other than that comment about removing 'b' from AllStdExts. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:39 + +static constexpr StringLiteral AllStdExts =

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng added inline comments. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:388 +const std::vector ) { + std::unique_ptr ISAInfo(new RISCVISAInfo()); + assert(XLen == 32 || XLen == 64); craig.topper wrote: > Use > > ``` > auto

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng updated this revision to Diff 379329. kito-cheng marked 11 inline comments as done. kito-cheng added a comment. Changes: - Address @craig.topper's comment - Add XLen to constructor of RISCVISAInfo. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

craig.topper added inline comments. Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:194 - if (MArch.startswith_insensitive("rv32")) { -// FIXME: parse `March` to find `D` extension properly -if (MArch.substr(4).contains_insensitive("d") || -

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng updated this revision to Diff 378729. kito-cheng added a comment. Changes: - Rebase to main - Remove b and zbproposedc. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105168/new/ https://reviews.llvm.org/D105168 Files:

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng added a comment. Herald added a subscriber: achieveartificialintelligence. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105168/new/ https://reviews.llvm.org/D105168 ___ cfe-commits

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng updated this revision to Diff 372939. kito-cheng added a comment. Changes: - Rebase to `main`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105168/new/ https://reviews.llvm.org/D105168 Files:

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng added a comment. Seems like conflict with D108187 , will update after testing :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105168/new/ https://reviews.llvm.org/D105168

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

luismarques added a comment. I was trying to put this patch through its paces but it no longer applies. Can you please rebase it? It seems this is nearly there. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105168/new/

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng updated this revision to Diff 371509. kito-cheng marked 2 inline comments as done. kito-cheng added a comment. Changes: - Address Jim's comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105168/new/ https://reviews.llvm.org/D105168

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

Jim added inline comments. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:416 +ExtensionInfoIterator->Version.Minor); + if (ExtName == "e") { +if (XLen != 32) Could this checking put before ISAInfo->addExtension...

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng marked 2 inline comments as done. kito-cheng added inline comments. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:387 +ExtensionInfoIterator->Version.Minor); + if (ExtName == "e") +HasE = true; Jim wrote: >

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng updated this revision to Diff 370174. kito-cheng marked an inline comment as done. kito-cheng added a comment. Changes: - Check feature combination is valid. - Change return type of parseFeatures to llvm::Expected>. - Set default extension version in getExtensionVersion.

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

Jim added inline comments. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:387 +ExtensionInfoIterator->Version.Minor); + if (ExtName == "e") +HasE = true; Does this need to check it is invalid if XLen is 64?

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng marked an inline comment as done. kito-cheng added inline comments. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:268 +// omitted from the version string. E.g., rv32i2p0, rv32i2, rv32i2p1. +static Error getExtensionVersion(StringRef MArch, StringRef Ext, StringRef In,

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng updated this revision to Diff 369364. kito-cheng added a comment. Changes: - Remove unused argument MArch for getExtensionVersion. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105168/new/ https://reviews.llvm.org/D105168 Files:

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

eopXD added inline comments. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:268 +// omitted from the version string. E.g., rv32i2p0, rv32i2, rv32i2p1. +static Error getExtensionVersion(StringRef MArch, StringRef Ext, StringRef In, + unsigned ,

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng added a comment. @asb Thanks for report that, there is no warning and build error when I build with GCC 7 (which is default compiler in Ubuntu 18.04), but I can reproduce that with clang 13, seems like I should switch my default compiler :P Repository: rG LLVM Github Monorepo

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng updated this revision to Diff 367704. kito-cheng added a comment. Changes: - Fix build warning and build error with clang Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105168/new/ https://reviews.llvm.org/D105168 Files:

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

asb added a comment. I'm getting a build error (building with clang 12.0.1): FAILED: lib/Support/CMakeFiles/LLVMSupport.dir/RISCVISAInfo.cpp.o /usr/bin/clang++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105168/new/ https://reviews.llvm.org/D105168 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng updated this revision to Diff 365941. kito-cheng added a comment. Changes: - Rename class, strip the `Info`: - `RISCVSupportedExtensionInfo` -> `RISCVSupportedExtension` - Rename variables: - `SupportedExtensionInfos` -> `SupportedExtensions` -

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng added inline comments. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:41 +static const RISCVSupportedExtensionInfo SupportedExtensionInfos[] = { +{"i", RISCVExtensionVersion{2, 0}}, {"e", RISCVExtensionVersion{1, 9}}, +{"m", RISCVExtensionVersion{2, 0}}, {"a",

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

jrtc27 added inline comments. Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:49-50 + /// Parse RISCV ISA info from arch string. + static std::unique_ptr + parseArchString(llvm::Error , StringRef Arch, + bool EnableExperimentalExtension,

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng added inline comments. Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:31 +public: + RISCVISAInfo() : XLen(0), FLen(0) {} + jrtc27 wrote: > Does Exts need initialising to be empty here? I can never remember std::map has default construct that

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng updated this revision to Diff 364758. kito-cheng marked an inline comment as done. kito-cheng added a comment. Changes: - Forbid copy ctor and operator= for RISCVISAInfo. - Move RISCVISAInfo's constructor to private. - Refine RISCVISAInfo::parse* and made they become static function.

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

jrtc27 added inline comments. Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:31 +public: + RISCVISAInfo() : XLen(0), FLen(0) {} + Does Exts need initialising to be empty here? I can never remember Comment at:

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng updated this revision to Diff 361915. kito-cheng marked an inline comment as done. kito-cheng added a comment. Changes: - Address @frasercrmck's comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105168/new/

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

frasercrmck added inline comments. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:127 + +bool RISCVISAInfo::isSupportedExtensionFeature(StringRef Ext) { + bool IsExperimental = stripExperimentalPrefix(Ext); This looks like a `find_if` if that'd make it any

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng marked an inline comment as done. kito-cheng added inline comments. Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:45 + + // Parse RISCV ISA info from arch string. + Error parse(StringRef Arch, bool EnableExperimentalExtension, jrtc27 wrote: >

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng updated this revision to Diff 361141. kito-cheng marked an inline comment as done. kito-cheng added a comment. Changes: - Rename RISCVISAInfo::parse to RISCVISAInfo::parseFeatures/RISCVISAInfo::parseArchString Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng marked 8 inline comments as done. kito-cheng added inline comments. Comment at: clang/test/Driver/riscv-arch.c:198-201 -// RUN: %clang -target riscv32-unknown-elf -march=rv32e -### %s \ -// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32E %s -// RV32E: error:

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng updated this revision to Diff 361137. kito-cheng added a comment. Changes: - New function RISCVISAInfo::isSupportedExtensionFeature - Remove parameter CheckExperimental from RISCVISAInfo::isSupportedExtension - Clean up obvious comments - Address @jrtc27's and @craig.topper's comments

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

jrtc27 added inline comments. Comment at: clang/test/Driver/riscv-abi.c:68 -// RUN: %clang -target riscv64-unknown-elf %s -### -o %t.o -march=rv64d -mabi=lp64d 2>&1 \ +// RUN: %clang -target riscv64-unknown-elf %s -### -o %t.o -march=rv64ifd -mabi=lp64d 2>&1 \ // RUN: |

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

craig.topper added inline comments. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:40 + +static const StringRef AllStdExts = "mafdqlcbjtpvn"; + Make this `static constexpr StringLiteral AllStdExts = "mafdqlcbjtpvn";` Comment at:

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng updated this revision to Diff 360842. kito-cheng marked an inline comment as done. kito-cheng added a comment. Changes: - Address @khchen's comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105168/new/

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng marked 2 inline comments as done. kito-cheng added inline comments. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:460 + +addExtension("e"); + } khchen wrote: > nit: add `break;` to avoid the implicit-fallthrough warning. Oh, thanks, it seems like

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

khchen added a comment. This all looks good to me except some tidy warning. wait for others comments. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:460 + +addExtension("e"); + } nit: add `break;` to avoid the implicit-fallthrough warning.

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng updated this revision to Diff 358193. kito-cheng added a comment. Changes: - Handle arch attribute emition in RISCVTargetStreamer.cpp, thanks @khchen for reminding me that! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105168/new/

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

khchen added inline comments. Comment at: clang/test/Driver/riscv-abi.c:68 -// RUN: %clang -target riscv64-unknown-elf %s -### -o %t.o -march=rv64d -mabi=lp64d 2>&1 \ +// RUN: %clang -target riscv64-unknown-elf %s -### -o %t.o -march=rv64ifd -mabi=lp64d 2>&1 \ // RUN: |

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng added inline comments. Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:42 + // keep entries in canonical order of extension. + typedef std::map + OrderedExtensionMap; craig.topper wrote: > Could this be a sorted vector? Would require a

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng updated this revision to Diff 358130. kito-cheng added a comment. Changes: - Using RISCVISAInfo in riscv::getRISCVABI - Allow rv32e for -march. - Address review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105168/new/

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

StephenFan added inline comments. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:115 + return make_filter_range(SupportedExtensionInfos, + [&](const RISCVSupportedExtensionInfo ) { + return ExtInfo.Name == ExtName;

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

craig.topper added inline comments. Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:42 + // keep entries in canonical order of extension. + typedef std::map + OrderedExtensionMap; Could this be a sorted vector? Would require a good spot to sort it

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

kito-cheng added inline comments. Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:195 if (MArch.startswith_insensitive("rv32")) { // FIXME: parse `March` to find `D` extension properly if (MArch.substr(4).contains_insensitive("d") || khchen

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

khchen added inline comments. Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:195 if (MArch.startswith_insensitive("rv32")) { // FIXME: parse `March` to find `D` extension properly if (MArch.substr(4).contains_insensitive("d") || I think

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

luismarques added inline comments. Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:1 +//===-- RISCVArchStringParser.h - RISCV Arch String Parser --*- C++ -*-===// +// Incorrect header name. Comment at: