[PATCH] D144696: [RISCV][NFC] Package version number information using RISCVExtensionVersion.

[yanming via Phabricator via cfe-commits]

ym1813382441 added inline comments.



Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:24
+  unsigned Minor;
+  bool operator==(const RISCVExtensionVersion &Vers) const {
+return this->Major == Vers.Major && this->Minor == Vers.Minor;

eopXD wrote:
> ym1813382441 wrote:
> > eopXD wrote:
> > > Does the structure already work as-is?
> > I've tested it and it works.
> I mean, without your modification to the structure, the original structure 
> already works. So I don't see how refactoring here can help you more on 
> supporting multiple versions of the same extension.
I just move RISCVExtensionVersion structure here, and remove RISCVExtensionInfo 
structure.
I think that RISCVExtensionInfo is redundant.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:190
+RISCVExtensionVersion Version) {
+  assert(!Exts.count(ExtName.str()) && "Extension already exists");
+  Exts[ExtName.str()] = Version;

eopXD wrote:
> ym1813382441 wrote:
> > eopXD wrote:
> > > Maybe handling this with a compiler error is better than an assertion 
> > > error.
> > Currently the compiler does not allow the same extension to appear more 
> > than once.
> > If it happens something has gone wrong.
> Yes. So we need to have an error instead of an assertion error to do this.
Are you expecting an error message from the compiler?
{F2038 size=full}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144696

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144696: [RISCV][NFC] Package version number information using RISCVExtensionVersion.

[yanming via Phabricator via cfe-commits]

ym1813382441 abandoned this revision.
ym1813382441 added a comment.

RISC-V International seem to have indicated they'd add new named extensions 
rather than add instructions to an existing extension in the future.
Related discussions at D115921 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144696

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144696: [RISCV][NFC] Package version number information using RISCVExtensionVersion.

[yanming via Phabricator via cfe-commits]

ym1813382441 created this revision.
Herald added subscribers: luke, VincentWu, vkmr, frasercrmck, evandro, 
luismarques, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, jocewei, 
PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, 
shiva0217, kito-cheng, niosHD, sabuasal, simoncook, johnrusso, rbar, asb, 
hiraditya, arichardson.
Herald added a project: All.
ym1813382441 requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits, pcwang-thead, eopXD, 
MaskRay.
Herald added projects: clang, LLVM.

This helps to support multiple version number extensions later.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D144696

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  llvm/include/llvm/Support/RISCVISAInfo.h
  llvm/lib/Support/RISCVISAInfo.cpp

Index: llvm/lib/Support/RISCVISAInfo.cpp
===
--- llvm/lib/Support/RISCVISAInfo.cpp
+++ llvm/lib/Support/RISCVISAInfo.cpp
@@ -23,11 +23,6 @@
 using namespace llvm;
 
 namespace {
-/// Represents the major and version number components of a RISC-V extension
-struct RISCVExtensionVersion {
-  unsigned Major;
-  unsigned Minor;
-};
 
 struct RISCVSupportedExtension {
   const char *Name;
@@ -190,13 +185,10 @@
   return std::nullopt;
 }
 
-void RISCVISAInfo::addExtension(StringRef ExtName, unsigned MajorVersion,
-unsigned MinorVersion) {
-  RISCVExtensionInfo Ext;
-  Ext.ExtName = ExtName.str();
-  Ext.MajorVersion = MajorVersion;
-  Ext.MinorVersion = MinorVersion;
-  Exts[ExtName.str()] = Ext;
+void RISCVISAInfo::addExtension(StringRef ExtName,
+RISCVExtensionVersion Version) {
+  assert(!Exts.count(ExtName.str()) && "Extension already exists");
+  Exts[ExtName.str()] = Version;
 }
 
 static StringRef getExtensionTypeDesc(StringRef Ext) {
@@ -247,11 +239,10 @@
  llvm::any_of(SupportedExperimentalExtensions, FindByName(Ext));
 }
 
-bool RISCVISAInfo::isSupportedExtension(StringRef Ext, unsigned MajorVersion,
-unsigned MinorVersion) {
+bool RISCVISAInfo::isSupportedExtension(StringRef Ext,
+RISCVExtensionVersion Version) {
   auto FindByNameAndVersion = [=](const RISCVSupportedExtension &ExtInfo) {
-return ExtInfo.Name == Ext && (MajorVersion == ExtInfo.Version.Major) &&
-   (MinorVersion == ExtInfo.Version.Minor);
+return ExtInfo.Name == Ext && (Version == ExtInfo.Version);
   };
   return llvm::any_of(SupportedExtensions, FindByNameAndVersion) ||
  llvm::any_of(SupportedExperimentalExtensions, FindByNameAndVersion);
@@ -381,13 +372,14 @@
 // Version number is divided into major and minor version numbers,
 // separated by a 'p'. If the minor version is 0 then 'p0' can be
 // omitted from the version string. E.g., rv32i2p0, rv32i2, rv32i2p1.
-static Error getExtensionVersion(StringRef Ext, StringRef In, unsigned &Major,
- unsigned &Minor, unsigned &ConsumeLength,
+static Error getExtensionVersion(StringRef Ext, StringRef In,
+ RISCVExtensionVersion &Version,
+ unsigned &ConsumeLength,
  bool EnableExperimentalExtension,
  bool ExperimentalExtensionVersionCheck) {
   StringRef MajorStr, MinorStr;
-  Major = 0;
-  Minor = 0;
+  Version.Major = 0;
+  Version.Minor = 0;
   ConsumeLength = 0;
   MajorStr = In.take_while(isDigit);
   In = In.substr(MajorStr.size());
@@ -404,12 +396,12 @@
 }
   }
 
-  if (!MajorStr.empty() && MajorStr.getAsInteger(10, Major))
+  if (!MajorStr.empty() && MajorStr.getAsInteger(10, Version.Major))
 return createStringError(
 errc::invalid_argument,
 "Failed to parse major version number for extension '" + Ext + "'");
 
-  if (!MinorStr.empty() && MinorStr.getAsInteger(10, Minor))
+  if (!MinorStr.empty() && MinorStr.getAsInteger(10, Version.Minor))
 return createStringError(
 errc::invalid_argument,
 "Failed to parse minor version number for extension '" + Ext + "'");
@@ -446,8 +438,7 @@
 }
 
 auto SupportedVers = *ExperimentalExtension;
-if (ExperimentalExtensionVersionCheck &&
-(Major != SupportedVers.Major || Minor != SupportedVers.Minor)) {
+if (ExperimentalExtensionVersionCheck && (Version != SupportedVers)) {
   std::string Error = "unsupported version number " + MajorStr.str();
   if (!MinorStr.empty())
 Error += "." + MinorStr.str();
@@ -466,15 +457,14 @@
 
   if (MajorStr.empty() && MinorStr.empty()) {
 if (auto DefaultVersion = findDefaultVersion(Ext)) {
-  Major = DefaultVersion->Major;
-  Minor = DefaultVersion->Minor;
+  Version = *DefaultVersion;
 }
 // No matter found or not, return success, assume other place will
 // verify.
 return Error::success();
   }

[PATCH] D144696: [RISCV][NFC] Package version number information using RISCVExtensionVersion.

[yanming via Phabricator via cfe-commits]

ym1813382441 added a comment.

I want to use arrays to maintain the supported version number of each 
extension, this patch in using the `find` algorithm helps to simplify the code.




Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:24
+  unsigned Minor;
+  bool operator==(const RISCVExtensionVersion &Vers) const {
+return this->Major == Vers.Major && this->Minor == Vers.Minor;

eopXD wrote:
> Does the structure already work as-is?
I've tested it and it works.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:190
+RISCVExtensionVersion Version) {
+  assert(!Exts.count(ExtName.str()) && "Extension already exists");
+  Exts[ExtName.str()] = Version;

eopXD wrote:
> Maybe handling this with a compiler error is better than an assertion error.
Currently the compiler does not allow the same extension to appear more than 
once.
If it happens something has gone wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144696

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144696: [RISCV][NFC] Package version number information using RISCVExtensionVersion.

[yanming via Phabricator via cfe-commits]

ym1813382441 marked 2 inline comments as done.
ym1813382441 added a comment.

F26630797: Screenshot from 2023-02-24 15-54-21.png 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144696

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144696: [RISCV][NFC] Package version number information using RISCVExtensionVersion.

[yanming via Phabricator via cfe-commits]

ym1813382441 added a comment.

In D144696#4149516 , @craig.topper 
wrote:

> Can you outline your full plan and why you want to do this. We need to see 
> the bigger picture.



In D144696#4149545 , @ym1813382441 
wrote:

> F26630797: Screenshot from 2023-02-24 15-54-21.png 
> 

I'm implementing this idea


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144696

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144696: [RISCV][NFC] Package version number information using RISCVExtensionVersion.

[yanming via Phabricator via cfe-commits]

ym1813382441 added a comment.

In D144696#4149560 , @craig.topper 
wrote:

> In D144696#4149545 , @ym1813382441 
> wrote:
>
>> F26630797: Screenshot from 2023-02-24 15-54-21.png 
>> 
>
> I meant what extensions are you planning to support multiple versions for and 
> why? How will this work in the assembler, code generator, and disassembler 
> which use SubtargetFeatures not RISCVISAInfo.

My intention is to support RVV0.71, since specific cpu's exist for the XuanTie 
C900 series. In LLVM use Feature to distinguish between different versions of 
extensions. I am considering how to solve this problem, using multiple `.td` 
files in the worst case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144696

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144696: [RISCV][NFC] Package version number information using RISCVExtensionVersion.

[yanming via Phabricator via cfe-commits]

ym1813382441 added a comment.

In D144696#4149706 , @craig.topper 
wrote:

> How much of the existing vector code in the backend including tablegen can be 
> reused for 0.71? How much of the intrinsic interface can be used? If we’re 
> talking about completely duplicating everything it is very unlikely we can 
> accept it. RISCVGenDAGISel.inc, for example, is already significantly larger 
> than any other target in tree. I don’t know how big it can get before it 
> becomes an issue.
>
> How many people are going to maintain this other version?

I will not submit RVV0.71 to the community, I just want to extend the interface 
to support multiple versions, this will help to solve the compatibility issue 
of different versions of the extension, this issue is common.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144696

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits