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

[Zixuan Wu via Phabricator via cfe-commits]

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 LAST ACTION
  https://reviews.llvm.org/D105168/new/

https://reviews.llvm.org/D105168

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

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

[Zixuan Wu via Phabricator via cfe-commits]

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 SubtargetFeature is enum value like ARM.td does, Feature.Value would be a 
uninitialization value, so I think there should be if condition like  
`Feature.Value < RISCV::NumSubtargetFeatures`, or there is a out-of-range visit.

```
def ProcXXX :
  SubtargetFeature<"", "RISCVProcFamily",
   "XXX", "XXX processors">;
```


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

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

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

In D105168#3071330 , @jrtc27 wrote:

> 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 option) Pass in a Twine -> `const char *` lambda that the 
>>> caller hooks up to Args.MakeArgString
>>
>> Good idea. I'll post a patch for that shortly.
>>
>>> 2. (probably the nicer option) Invent our own MakeArgString that allocates 
>>> from storage owned by RISCVISAInfo itself; lots of ways you can do that
>>
>> I don't think the RISCVISAInfo objects lives long enough for that. It only 
>> lives for the duration of the function that calls toFeatures, but the 
>> Features vector is returned from that function.
>
> Hm, I see. We could also just have a global cache of heap-allocated copies of 
> these strings, I guess; after all it's not all that different from the tables 
> of strings we already have, just lazily built up at run time (can even store 
> them in a lazily-initialised field of each RISCVExtensionInfo). Or we just 
> stick to the slightly-ugly lambda approach, unless anyone else has bright 
> ideas for a cleaner interface and implementation.

Lambda approach here https://reviews.llvm.org/D112032


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

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

[Jessica Clarke via Phabricator via cfe-commits]

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 option) Pass in a Twine -> `const char *` lambda that the 
>> caller hooks up to Args.MakeArgString
>
> Good idea. I'll post a patch for that shortly.
>
>> 2. (probably the nicer option) Invent our own MakeArgString that allocates 
>> from storage owned by RISCVISAInfo itself; lots of ways you can do that
>
> I don't think the RISCVISAInfo objects lives long enough for that. It only 
> lives for the duration of the function that calls toFeatures, but the 
> Features vector is returned from that function.

Hm, I see. We could also just have a global cache of heap-allocated copies of 
these strings, I guess; after all it's not all that different from the tables 
of strings we already have, just lazily built up at run time. Or we just stick 
to the slightly-ugly lambda approach, unless anyone else has bright ideas for a 
cleaner interface and implementation.


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

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

[Craig Topper via Phabricator via cfe-commits]

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 Args.MakeArgString

Good idea. I'll post a patch for that shortly.

> 2. (probably the nicer option) Invent our own MakeArgString that allocates 
> from storage owned by RISCVISAInfo itself; lots of ways you can do that

I don't think the RISCVISAInfo objects lives long enough for that. It only 
lives for the duration of the function that calls toFeatures, but the Features 
vector is returned from that function.

> Adding a new library just so we can use MakeArgString seems overkill, even if 
> there are other things that perhaps belong in something a bit less shared 
> than Support.




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

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

[Jessica Clarke via Phabricator via cfe-commits]

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 
storage owned by RISCVISAInfo itself; lots of ways you can do that

Adding a new library just so we can use MakeArgString seems overkill, even if 
there are other things that perhaps belong in something a bit less shared than 
Support.


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

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

[Raphael Isemann via Phabricator via cfe-commits]

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 
>> 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 "llvm/Option/ArgList.h"
>>^~~
>>   llvm/lib/Support/TargetParser.cpp:14:10: fatal error: could not build 
>> module 'LLVM_Utils'
>>   #include "llvm/Support/TargetParser.h"
>>^
>>
>> `Support` headers can't include `Option` headers (as `Option` depends on 
>> `Support` already, so that would be a cyclic dependency). I believe folks 
>> here are in the US time zone, so I went ahead and replaced the header 
>> include with a forward decl to unbreak the bots (see 
>> de4d2f80b75e2a1e4b0ac5c25e20f20839633688 
>>  )
>>
>> FWIW, I think there is a better place than `Support` for this new class. 
>> `Support` is a dependency of nearly every single LLVM component, but this 
>> class seems to be used by a handful of files. Could we maybe move this to 
>> some other/new part of LLVM (and make the few components that need it depend 
>> on that)?
>
> Thanks for the header fix. I think we also need to fix the library circular 
> dependency that still exists. The Support library should not use anything 
> from the Option library. Maybe we can partition the function some way that 
> moves the MakeArgStr back into the clang driver code.
>
> I don't know if we have a better library for this new class. I think Support 
> is the only common library between the clang driver and LLVM's MC layer. I 
> supposed we could create a new library, but that might be a hard sell for one 
> file.

IIRC there are some other target-specific classes in Support for the same 
reason. Maybe we can make something such as `TargetSupport` or `SharedTarget`?


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

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

[Craig Topper via Phabricator via cfe-commits]

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:
>   llvm/include/llvm/Support/RISCVISAInfo.h:15:10: fatal error: could not 
> build module 'LLVM_Option'
>   #include "llvm/Option/ArgList.h"
>^~~
>   llvm/lib/Support/TargetParser.cpp:14:10: fatal error: could not build 
> module 'LLVM_Utils'
>   #include "llvm/Support/TargetParser.h"
>^
>
> `Support` headers can't include `Option` headers (as `Option` depends on 
> `Support` already, so that would be a cyclic dependency). I believe folks 
> here are in the US time zone, so I went ahead and replaced the header include 
> with a forward decl to unbreak the bots (see 
> de4d2f80b75e2a1e4b0ac5c25e20f20839633688 
>  )
>
> FWIW, I think there is a better place than `Support` for this new class. 
> `Support` is a dependency of nearly every single LLVM component, but this 
> class seems to be used by a handful of files. Could we maybe move this to 
> some other/new part of LLVM (and make the few components that need it depend 
> on that)?

Thanks for the header fix. I think we also need to fix the library circular 
dependency that still exists. The Support library should not use anything from 
the Option library. Maybe we can partition the function some way that moves the 
MakeArgStr back into the clang driver code.

I don't know if we have a better library for this new class. I think Support is 
the only common library between the clang driver and LLVM's MC layer. I 
supposed we could create a new library, but that might be a hard sell for one 
file.


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

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

[Raphael Isemann via Phabricator via cfe-commits]

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 "llvm/Option/ArgList.h"
   ^~~
  llvm/lib/Support/TargetParser.cpp:14:10: fatal error: could not build module 
'LLVM_Utils'
  #include "llvm/Support/TargetParser.h"
   ^

`Support` headers can't include `Option` headers (as `Option` depends on 
`Support` already, so that would be a cyclic dependency). I believe folks here 
are in the US time zone, so I went ahead and replaced the header include with a 
forward decl to unbreak the bots (see de4d2f80b75e2a1e4b0ac5c25e20f20839633688 
 )

FWIW, I think there is a better place than `Support` for this new class. 
`Support` is a dependency of nearly every single LLVM component, but this class 
seems to be used by a handful of files. Could we maybe move this to some 
other/new part of LLVM (and make the few components that need it depend on 
that)?


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

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

[Adrian Kuegel via Phabricator via cfe-commits]

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 to 
Support?
This introduces a dependency cycle, since Option already depends on Support.


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

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

[Kito Cheng via Phabricator via cfe-commits]

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 for sync with ISA manual, like other unsupported 
> extension like `J`, `T` and ``N`, we remove `B` from `SupportedExtensions`, 
> so keep this should be harmless.
Oh, and it also used for compute the canonical order for `z*` extensions.


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

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

[Kito Cheng via Phabricator via cfe-commits]

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

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

[Phabricator via cfe-commits]

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 commit:
  https://reviews.llvm.org/D105168?vs=379329=380228#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168

Files:
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Driver/ToolChains/Arch/RISCV.cpp
  clang/test/Driver/riscv-abi.c
  clang/test/Driver/riscv-arch.c
  clang/test/Driver/riscv-features.c
  llvm/include/llvm/Support/RISCVISAInfo.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/invalid-attribute.s

Index: llvm/test/MC/RISCV/invalid-attribute.s
===
--- llvm/test/MC/RISCV/invalid-attribute.s
+++ llvm/test/MC/RISCV/invalid-attribute.s
@@ -7,10 +7,10 @@
 # RUN: not llvm-mc %s -triple=riscv64 -filetype=asm 2>&1 | FileCheck %s
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: bad arch string foo
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,g}
 
 .attribute arch, "rv32i2p0_y2p0"
-# CHECK: [[@LINE-1]]:18: error: bad arch string y2p0
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p0_y2p0', invalid standard user-level extension 'y'
 
 .attribute stack_align, "16"
 # CHECK: [[@LINE-1]]:25: error: expected numeric constant
Index: llvm/test/MC/RISCV/attribute-with-insts.s
===
--- llvm/test/MC/RISCV/attribute-with-insts.s
+++ llvm/test/MC/RISCV/attribute-with-insts.s
@@ -10,7 +10,7 @@
 # RUN:   | llvm-objdump --triple=riscv64 -d -M no-aliases - \
 # RUN:   | FileCheck -check-prefix=CHECK-INST %s
 
-.attribute arch, "rv64i2p0_m2p0_a2p0_d2p0_c2p0"
+.attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 # CHECK-INST: lr.w t0, (t1)
 lr.w t0, (t1)
Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -9,9 +9,6 @@
 .attribute arch, "rv32i2"
 # CHECK: attribute  5, "rv32i2p0"
 
-.attribute arch, "rv32i2p"
-# CHECK: attribute  5, "rv32i2p0"
-
 .attribute arch, "rv32i2p0"
 # CHECK: attribute  5, "rv32i2p0"
 
@@ -33,11 +30,11 @@
 .attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
-.attribute arch, "rv32ima2p_fdc"
+.attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 .attribute arch, "rv32iv"
-# CHECK: attribute  5, "rv32i2p0_v0p10"
+# CHECK: attribute  5, "rv32i2p0_v0p10_zvlsseg0p10"
 
 .attribute arch, "rv32izba"
 # CHECK: attribute  5, "rv32i2p0_zba1p0"
Index: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
===
--- llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
+++ llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
@@ -11,9 +11,11 @@
 //===--===//
 
 #include "RISCVTargetStreamer.h"
+#include "RISCVBaseInfo.h"
 #include "RISCVMCTargetDesc.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 
 using namespace llvm;
 
@@ -43,53 +45,19 @@
   else
 emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);
 
-  std::string Arch = "rv32";
-  if (STI.hasFeature(RISCV::Feature64Bit))
-Arch = "rv64";
-  if (STI.hasFeature(RISCV::FeatureRV32E))
-Arch += "e1p9";
-  else
-Arch += "i2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtM))
-Arch += "_m2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtA))
-Arch += "_a2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtF))
-Arch += "_f2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtD))
-Arch += "_d2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtC))
-Arch += "_c2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtV))
-Arch += "_v0p10";
-  if (STI.hasFeature(RISCV::FeatureStdExtZfh))
-Arch += "_zfh0p1";
-  if (STI.hasFeature(RISCV::FeatureStdExtZba))
-Arch += "_zba1p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtZbb))
-Arch += "_zbb1p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtZbc))
-Arch += "_zbc1p0";
-  if 

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

[Kito Cheng via Phabricator via cfe-commits]

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 manual, like other unsupported 
extension like `J`, `T` and ``N`, we remove `B` from `SupportedExtensions`, so 
keep this should be harmless.



Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2089
 else
   return Error(ValueExprLoc, "bad arch string " + Arch);
   }

khchen wrote:
> maybe we could have a NFC patch later to add 
> ```
> // Emit the arch string if needed.
> if (!IsIntegerValue)
>   getTargetStreamer().emitTextAttribute(Tag, ISAInfo->toString());
> ```
> here to reuse `ISAInfo`, and remove 2098~2117 code.
> 
Good point, thanks :)
Let improve that by following patch.


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

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

[Zakk Chen via Phabricator via cfe-commits]

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 (!IsIntegerValue)
  getTargetStreamer().emitTextAttribute(Tag, ISAInfo->toString());
```
here to reuse `ISAInfo`, and remove 2098~2117 code.



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

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

[Craig Topper via Phabricator via cfe-commits]

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 = "mafdqlcbjtpvn";
+

'b' shouldn't be in this list anymore?



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:388
+const std::vector ) {
+  std::unique_ptr ISAInfo(new RISCVISAInfo());
+  assert(XLen == 32 || XLen == 64);

kito-cheng wrote:
> craig.topper wrote:
> > Use
> > 
> > ```
> > auto ISAInfo = std::make_unique()
> > ```
> `std::make_unique()` require a `public` constructor, but I 
> would prefer keep that in `private`.
I agree with keeping the constructor private. I didn't realize make_unique had 
that restriction.


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

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

[Kito Cheng via Phabricator via cfe-commits]

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 ISAInfo = std::make_unique()
> ```
`std::make_unique()` require a `public` constructor, but I would 
prefer keep that in `private`.


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

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

[Kito Cheng via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D105168/new/

https://reviews.llvm.org/D105168

Files:
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Driver/ToolChains/Arch/RISCV.cpp
  clang/test/Driver/riscv-abi.c
  clang/test/Driver/riscv-arch.c
  clang/test/Driver/riscv-features.c
  llvm/include/llvm/Support/RISCVISAInfo.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/invalid-attribute.s

Index: llvm/test/MC/RISCV/invalid-attribute.s
===
--- llvm/test/MC/RISCV/invalid-attribute.s
+++ llvm/test/MC/RISCV/invalid-attribute.s
@@ -7,10 +7,10 @@
 # RUN: not llvm-mc %s -triple=riscv64 -filetype=asm 2>&1 | FileCheck %s
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: bad arch string foo
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,g}
 
 .attribute arch, "rv32i2p0_y2p0"
-# CHECK: [[@LINE-1]]:18: error: bad arch string y2p0
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p0_y2p0', invalid standard user-level extension 'y'
 
 .attribute stack_align, "16"
 # CHECK: [[@LINE-1]]:25: error: expected numeric constant
Index: llvm/test/MC/RISCV/attribute-with-insts.s
===
--- llvm/test/MC/RISCV/attribute-with-insts.s
+++ llvm/test/MC/RISCV/attribute-with-insts.s
@@ -10,7 +10,7 @@
 # RUN:   | llvm-objdump --triple=riscv64 -d -M no-aliases - \
 # RUN:   | FileCheck -check-prefix=CHECK-INST %s
 
-.attribute arch, "rv64i2p0_m2p0_a2p0_d2p0_c2p0"
+.attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 # CHECK-INST: lr.w t0, (t1)
 lr.w t0, (t1)
Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -9,9 +9,6 @@
 .attribute arch, "rv32i2"
 # CHECK: attribute  5, "rv32i2p0"
 
-.attribute arch, "rv32i2p"
-# CHECK: attribute  5, "rv32i2p0"
-
 .attribute arch, "rv32i2p0"
 # CHECK: attribute  5, "rv32i2p0"
 
@@ -33,11 +30,11 @@
 .attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
-.attribute arch, "rv32ima2p_fdc"
+.attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 .attribute arch, "rv32iv"
-# CHECK: attribute  5, "rv32i2p0_v0p10"
+# CHECK: attribute  5, "rv32i2p0_v0p10_zvlsseg0p10"
 
 .attribute arch, "rv32izba"
 # CHECK: attribute  5, "rv32i2p0_zba0p93"
Index: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
===
--- llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
+++ llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
@@ -11,9 +11,11 @@
 //===--===//
 
 #include "RISCVTargetStreamer.h"
+#include "RISCVBaseInfo.h"
 #include "RISCVMCTargetDesc.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 
 using namespace llvm;
 
@@ -43,53 +45,19 @@
   else
 emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);
 
-  std::string Arch = "rv32";
-  if (STI.hasFeature(RISCV::Feature64Bit))
-Arch = "rv64";
-  if (STI.hasFeature(RISCV::FeatureRV32E))
-Arch += "e1p9";
-  else
-Arch += "i2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtM))
-Arch += "_m2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtA))
-Arch += "_a2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtF))
-Arch += "_f2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtD))
-Arch += "_d2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtC))
-Arch += "_c2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtV))
-Arch += "_v0p10";
-  if (STI.hasFeature(RISCV::FeatureStdExtZfh))
-Arch += "_zfh0p1";
-  if (STI.hasFeature(RISCV::FeatureStdExtZba))
-Arch += "_zba0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtZbb))
-Arch += "_zbb0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtZbc))
-Arch += "_zbc0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtZbe))
-Arch += "_zbe0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtZbf))
-Arch += "_zbf0p93";
-  if 

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

[Craig Topper via Phabricator via cfe-commits]

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") ||
-MArch.startswith_insensitive("rv32g"))
-  return "ilp32d";
-else if (MArch.startswith_insensitive("rv32e"))
-  return "ilp32e";
-else
-  return "ilp32";
-  } else if (MArch.startswith_insensitive("rv64")) {
-// FIXME: parse `March` to find `D` extension properly
-if (MArch.substr(4).contains_insensitive("d") ||
-MArch.startswith_insensitive("rv64g"))
-  return "lp64d";
-else
-  return "lp64";
+  auto ParseResult = llvm::RISCVISAInfo::parseArchString(Arch, true);
+  if (!ParseResult) {

Please add an inline comment for what "true" represents.



Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:206
+return "ilp32d";
+  else if (HasE)
+return "ilp32e";

Drop else after return



Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:208
+return "ilp32e";
+  else
+return "ilp32";

here too



Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:213
+return "lp64d";
+  else
+return "lp64";

Here too



Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:216
+}
   }
 

Should we have an llvm_unreachable here for unknown values of XLen?



Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:43
+
+  /// OrderedExtensionMap is a StringMap-like container, but specialized to
+  /// keep entries in canonical order of extension.

I'm not sure the comparison to StringMap is helpful here. StringMap is an 
unordered map that stores strings in the same allocation as the value. This 
std::map is ordered and doesn't store the strings in the same allocation.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:286
+ bool ExperimentalExtensionVersionCheck) {
+  std::string MajorStr, MinorStr;
+  Major = 0;

Do MajorStr and MinorStr need to be std::strings or can they be StringRefs? 
Looks like they are either empty or a subset of In. So there shouldn't be a 
lifetime issue?



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:352
+Error += "." + MinorStr;
+  Error += " for experimental extension (this compiler supports " +
+   utostr(SupportedVers.Major) + "." + utostr(SupportedVers.Minor) 
+

Should this message say which extension the error applies to?



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:388
+const std::vector ) {
+  std::unique_ptr ISAInfo(new RISCVISAInfo());
+  assert(XLen == 32 || XLen == 64);

Use

```
auto ISAInfo = std::make_unique()
```



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:390
+  assert(XLen == 32 || XLen == 64);
+  ISAInfo->XLen = XLen;
+

Can we make XLen part of the RISCVISAInfo constructor? Seems like we know that 
early enough.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:440
+  bool ExperimentalExtensionVersionCheck) {
+  std::unique_ptr ISAInfo(new RISCVISAInfo());
+  // RISC-V ISA strings must be lowercase.

Can we delay constructing this until after the first 2 error checks? Then we 
could pass XLen to the constructor.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:709
+
+  if (XLen == 32)
+Arch << "rv32";

Would this better as

```
Arch << "rv" << XLen;
```

That makes it future proof to a hypothetical rv128 someday.


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

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

[Kito Cheng via Phabricator via cfe-commits]

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:
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Driver/ToolChains/Arch/RISCV.cpp
  clang/test/Driver/riscv-abi.c
  clang/test/Driver/riscv-arch.c
  clang/test/Driver/riscv-features.c
  llvm/include/llvm/Support/RISCVISAInfo.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/invalid-attribute.s

Index: llvm/test/MC/RISCV/invalid-attribute.s
===
--- llvm/test/MC/RISCV/invalid-attribute.s
+++ llvm/test/MC/RISCV/invalid-attribute.s
@@ -7,10 +7,10 @@
 # RUN: not llvm-mc %s -triple=riscv64 -filetype=asm 2>&1 | FileCheck %s
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: bad arch string foo
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,g}
 
 .attribute arch, "rv32i2p0_y2p0"
-# CHECK: [[@LINE-1]]:18: error: bad arch string y2p0
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p0_y2p0', invalid standard user-level extension 'y'
 
 .attribute stack_align, "16"
 # CHECK: [[@LINE-1]]:25: error: expected numeric constant
Index: llvm/test/MC/RISCV/attribute-with-insts.s
===
--- llvm/test/MC/RISCV/attribute-with-insts.s
+++ llvm/test/MC/RISCV/attribute-with-insts.s
@@ -10,7 +10,7 @@
 # RUN:   | llvm-objdump --triple=riscv64 -d -M no-aliases - \
 # RUN:   | FileCheck -check-prefix=CHECK-INST %s
 
-.attribute arch, "rv64i2p0_m2p0_a2p0_d2p0_c2p0"
+.attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 # CHECK-INST: lr.w t0, (t1)
 lr.w t0, (t1)
Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -9,9 +9,6 @@
 .attribute arch, "rv32i2"
 # CHECK: attribute  5, "rv32i2p0"
 
-.attribute arch, "rv32i2p"
-# CHECK: attribute  5, "rv32i2p0"
-
 .attribute arch, "rv32i2p0"
 # CHECK: attribute  5, "rv32i2p0"
 
@@ -33,11 +30,11 @@
 .attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
-.attribute arch, "rv32ima2p_fdc"
+.attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 .attribute arch, "rv32iv"
-# CHECK: attribute  5, "rv32i2p0_v0p10"
+# CHECK: attribute  5, "rv32i2p0_v0p10_zvlsseg0p10"
 
 .attribute arch, "rv32izba"
 # CHECK: attribute  5, "rv32i2p0_zba0p93"
Index: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
===
--- llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
+++ llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
@@ -11,9 +11,11 @@
 //===--===//
 
 #include "RISCVTargetStreamer.h"
+#include "RISCVBaseInfo.h"
 #include "RISCVMCTargetDesc.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 
 using namespace llvm;
 
@@ -43,53 +45,19 @@
   else
 emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);
 
-  std::string Arch = "rv32";
-  if (STI.hasFeature(RISCV::Feature64Bit))
-Arch = "rv64";
-  if (STI.hasFeature(RISCV::FeatureRV32E))
-Arch += "e1p9";
-  else
-Arch += "i2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtM))
-Arch += "_m2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtA))
-Arch += "_a2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtF))
-Arch += "_f2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtD))
-Arch += "_d2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtC))
-Arch += "_c2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtV))
-Arch += "_v0p10";
-  if (STI.hasFeature(RISCV::FeatureStdExtZfh))
-Arch += "_zfh0p1";
-  if (STI.hasFeature(RISCV::FeatureStdExtZba))
-Arch += "_zba0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtZbb))
-Arch += "_zbb0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtZbc))
-Arch += "_zbc0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtZbe))
-Arch += "_zbe0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtZbf))
-Arch += "_zbf0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtZbm))
-Arch += "_zbm0p93";
-  if 

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

[Kito Cheng via Phabricator via cfe-commits]

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

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

[Kito Cheng via Phabricator via cfe-commits]

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:
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Driver/ToolChains/Arch/RISCV.cpp
  clang/test/Driver/riscv-abi.c
  clang/test/Driver/riscv-arch.c
  clang/test/Driver/riscv-features.c
  llvm/include/llvm/Support/RISCVISAInfo.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/invalid-attribute.s

Index: llvm/test/MC/RISCV/invalid-attribute.s
===
--- llvm/test/MC/RISCV/invalid-attribute.s
+++ llvm/test/MC/RISCV/invalid-attribute.s
@@ -7,10 +7,10 @@
 # RUN: not llvm-mc %s -triple=riscv64 -filetype=asm 2>&1 | FileCheck %s
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: bad arch string foo
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,g}
 
 .attribute arch, "rv32i2p0_y2p0"
-# CHECK: [[@LINE-1]]:18: error: bad arch string y2p0
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p0_y2p0', invalid standard user-level extension 'y'
 
 .attribute stack_align, "16"
 # CHECK: [[@LINE-1]]:25: error: expected numeric constant
Index: llvm/test/MC/RISCV/attribute-with-insts.s
===
--- llvm/test/MC/RISCV/attribute-with-insts.s
+++ llvm/test/MC/RISCV/attribute-with-insts.s
@@ -10,7 +10,7 @@
 # RUN:   | llvm-objdump --triple=riscv64 -d -M no-aliases - \
 # RUN:   | FileCheck -check-prefix=CHECK-INST %s
 
-.attribute arch, "rv64i2p0_m2p0_a2p0_d2p0_c2p0"
+.attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 # CHECK-INST: lr.w t0, (t1)
 lr.w t0, (t1)
Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -9,9 +9,6 @@
 .attribute arch, "rv32i2"
 # CHECK: attribute  5, "rv32i2p0"
 
-.attribute arch, "rv32i2p"
-# CHECK: attribute  5, "rv32i2p0"
-
 .attribute arch, "rv32i2p0"
 # CHECK: attribute  5, "rv32i2p0"
 
@@ -33,14 +30,14 @@
 .attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
-.attribute arch, "rv32ima2p_fdc"
+.attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 .attribute arch, "rv32ib"
 # CHECK: attribute  5, "rv32i2p0_b0p93_zba0p93_zbb0p93_zbc0p93_zbe0p93_zbf0p93_zbm0p93_zbp0p93_zbr0p93_zbs0p93_zbt0p93"
 
 .attribute arch, "rv32iv"
-# CHECK: attribute  5, "rv32i2p0_v0p10"
+# CHECK: attribute  5, "rv32i2p0_v0p10_zvlsseg0p10"
 
 .attribute arch, "rv32izba"
 # CHECK: attribute  5, "rv32i2p0_zba0p93"
Index: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
===
--- llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
+++ llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
@@ -11,9 +11,11 @@
 //===--===//
 
 #include "RISCVTargetStreamer.h"
+#include "RISCVBaseInfo.h"
 #include "RISCVMCTargetDesc.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 
 using namespace llvm;
 
@@ -43,57 +45,19 @@
   else
 emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);
 
-  std::string Arch = "rv32";
-  if (STI.hasFeature(RISCV::Feature64Bit))
-Arch = "rv64";
-  if (STI.hasFeature(RISCV::FeatureRV32E))
-Arch += "e1p9";
-  else
-Arch += "i2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtM))
-Arch += "_m2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtA))
-Arch += "_a2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtF))
-Arch += "_f2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtD))
-Arch += "_d2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtC))
-Arch += "_c2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtB))
-Arch += "_b0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtV))
-Arch += "_v0p10";
-  if (STI.hasFeature(RISCV::FeatureStdExtZfh))
-Arch += "_zfh0p1";
-  if (STI.hasFeature(RISCV::FeatureStdExtZba))
-Arch += "_zba0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtZbb))
-Arch += "_zbb0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtZbc))
-Arch += "_zbc0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtZbe))
-Arch 

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

[Kito Cheng via Phabricator via cfe-commits]

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

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

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

[Luís Marques via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D105168

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

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

[Kito Cheng via Phabricator via cfe-commits]

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

Files:
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Driver/ToolChains/Arch/RISCV.cpp
  clang/test/Driver/riscv-abi.c
  clang/test/Driver/riscv-arch.c
  clang/test/Driver/riscv-features.c
  llvm/include/llvm/Support/RISCVISAInfo.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/invalid-attribute.s

Index: llvm/test/MC/RISCV/invalid-attribute.s
===
--- llvm/test/MC/RISCV/invalid-attribute.s
+++ llvm/test/MC/RISCV/invalid-attribute.s
@@ -7,10 +7,10 @@
 # RUN: not llvm-mc %s -triple=riscv64 -filetype=asm 2>&1 | FileCheck %s
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: bad arch string foo
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,g}
 
 .attribute arch, "rv32i2p0_y2p0"
-# CHECK: [[@LINE-1]]:18: error: bad arch string y2p0
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p0_y2p0', invalid standard user-level extension 'y'
 
 .attribute stack_align, "16"
 # CHECK: [[@LINE-1]]:25: error: expected numeric constant
Index: llvm/test/MC/RISCV/attribute-with-insts.s
===
--- llvm/test/MC/RISCV/attribute-with-insts.s
+++ llvm/test/MC/RISCV/attribute-with-insts.s
@@ -10,7 +10,7 @@
 # RUN:   | llvm-objdump --triple=riscv64 -d -M no-aliases - \
 # RUN:   | FileCheck -check-prefix=CHECK-INST %s
 
-.attribute arch, "rv64i2p0_m2p0_a2p0_d2p0_c2p0"
+.attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 # CHECK-INST: lr.w t0, (t1)
 lr.w t0, (t1)
Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -9,9 +9,6 @@
 .attribute arch, "rv32i2"
 # CHECK: attribute  5, "rv32i2p0"
 
-.attribute arch, "rv32i2p"
-# CHECK: attribute  5, "rv32i2p0"
-
 .attribute arch, "rv32i2p0"
 # CHECK: attribute  5, "rv32i2p0"
 
@@ -33,14 +30,14 @@
 .attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
-.attribute arch, "rv32ima2p_fdc"
+.attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 .attribute arch, "rv32ib"
 # CHECK: attribute  5, "rv32i2p0_b0p93_zba0p93_zbb0p93_zbc0p93_zbe0p93_zbf0p93_zbm0p93_zbp0p93_zbr0p93_zbs0p93_zbt0p93"
 
 .attribute arch, "rv32iv"
-# CHECK: attribute  5, "rv32i2p0_v0p10"
+# CHECK: attribute  5, "rv32i2p0_v0p10_zvlsseg0p10"
 
 .attribute arch, "rv32izba"
 # CHECK: attribute  5, "rv32i2p0_zba0p93"
Index: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
===
--- llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
+++ llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
@@ -11,9 +11,11 @@
 //===--===//
 
 #include "RISCVTargetStreamer.h"
+#include "RISCVBaseInfo.h"
 #include "RISCVMCTargetDesc.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 
 using namespace llvm;
 
@@ -43,57 +45,19 @@
   else
 emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);
 
-  std::string Arch = "rv32";
-  if (STI.hasFeature(RISCV::Feature64Bit))
-Arch = "rv64";
-  if (STI.hasFeature(RISCV::FeatureRV32E))
-Arch += "e1p9";
-  else
-Arch += "i2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtM))
-Arch += "_m2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtA))
-Arch += "_a2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtF))
-Arch += "_f2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtD))
-Arch += "_d2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtC))
-Arch += "_c2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtB))
-Arch += "_b0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtV))
-Arch += "_v0p10";
-  if (STI.hasFeature(RISCV::FeatureExtZfh))
-Arch += "_zfh0p1";
-  if (STI.hasFeature(RISCV::FeatureExtZba))
-Arch += "_zba0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbb))
-Arch += "_zbb0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbc))
-Arch += "_zbc0p93";
-  if 

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

[Jim Lin via Phabricator via cfe-commits]

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...


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

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

[Kito Cheng via Phabricator via cfe-commits]

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:
> Does this need to check it is invalid if XLen is 64?
Add check, and parseFeatures might return Error now.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:394
+  if (!HasE)
+ISAInfo->addExtension("i", 2, 0);
+

Jim wrote:
> Why not get the version of i from SupportedExtensions?
Updated :)


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

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

[Kito Cheng via Phabricator via cfe-commits]

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168

Files:
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Driver/ToolChains/Arch/RISCV.cpp
  clang/test/Driver/riscv-abi.c
  clang/test/Driver/riscv-arch.c
  clang/test/Driver/riscv-features.c
  llvm/include/llvm/Support/RISCVISAInfo.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/invalid-attribute.s

Index: llvm/test/MC/RISCV/invalid-attribute.s
===
--- llvm/test/MC/RISCV/invalid-attribute.s
+++ llvm/test/MC/RISCV/invalid-attribute.s
@@ -7,10 +7,10 @@
 # RUN: not llvm-mc %s -triple=riscv64 -filetype=asm 2>&1 | FileCheck %s
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: bad arch string foo
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,g}
 
 .attribute arch, "rv32i2p0_y2p0"
-# CHECK: [[@LINE-1]]:18: error: bad arch string y2p0
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p0_y2p0', invalid standard user-level extension 'y'
 
 .attribute stack_align, "16"
 # CHECK: [[@LINE-1]]:25: error: expected numeric constant
Index: llvm/test/MC/RISCV/attribute-with-insts.s
===
--- llvm/test/MC/RISCV/attribute-with-insts.s
+++ llvm/test/MC/RISCV/attribute-with-insts.s
@@ -10,7 +10,7 @@
 # RUN:   | llvm-objdump --triple=riscv64 -d -M no-aliases - \
 # RUN:   | FileCheck -check-prefix=CHECK-INST %s
 
-.attribute arch, "rv64i2p0_m2p0_a2p0_d2p0_c2p0"
+.attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 # CHECK-INST: lr.w t0, (t1)
 lr.w t0, (t1)
Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -9,9 +9,6 @@
 .attribute arch, "rv32i2"
 # CHECK: attribute  5, "rv32i2p0"
 
-.attribute arch, "rv32i2p"
-# CHECK: attribute  5, "rv32i2p0"
-
 .attribute arch, "rv32i2p0"
 # CHECK: attribute  5, "rv32i2p0"
 
@@ -33,14 +30,14 @@
 .attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
-.attribute arch, "rv32ima2p_fdc"
+.attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 .attribute arch, "rv32ib"
 # CHECK: attribute  5, "rv32i2p0_b0p93_zba0p93_zbb0p93_zbc0p93_zbe0p93_zbf0p93_zbm0p93_zbp0p93_zbr0p93_zbs0p93_zbt0p93"
 
 .attribute arch, "rv32iv"
-# CHECK: attribute  5, "rv32i2p0_v0p10"
+# CHECK: attribute  5, "rv32i2p0_v0p10_zvlsseg0p10"
 
 .attribute arch, "rv32izba"
 # CHECK: attribute  5, "rv32i2p0_zba0p93"
Index: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
===
--- llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
+++ llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
@@ -11,9 +11,11 @@
 //===--===//
 
 #include "RISCVTargetStreamer.h"
+#include "RISCVBaseInfo.h"
 #include "RISCVMCTargetDesc.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 
 using namespace llvm;
 
@@ -43,57 +45,19 @@
   else
 emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);
 
-  std::string Arch = "rv32";
-  if (STI.hasFeature(RISCV::Feature64Bit))
-Arch = "rv64";
-  if (STI.hasFeature(RISCV::FeatureRV32E))
-Arch += "e1p9";
-  else
-Arch += "i2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtM))
-Arch += "_m2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtA))
-Arch += "_a2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtF))
-Arch += "_f2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtD))
-Arch += "_d2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtC))
-Arch += "_c2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtB))
-Arch += "_b0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtV))
-Arch += "_v0p10";
-  if (STI.hasFeature(RISCV::FeatureExtZfh))
-Arch += "_zfh0p1";
-  if (STI.hasFeature(RISCV::FeatureExtZba))
-Arch += "_zba0p93";
-  if 

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

[Jim Lin via Phabricator via cfe-commits]

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?



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:394
+  if (!HasE)
+ISAInfo->addExtension("i", 2, 0);
+

Why not get the version of i from SupportedExtensions?


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

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

[Kito Cheng via Phabricator via cfe-commits]

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,
+ unsigned , unsigned ,

eopXD wrote:
> For naming consistency with `parseArchString`, maybe rename `MArch` to `Arch`?
It turns out the argument isn't used anywhere...so I remove that.


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

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

[Kito Cheng via Phabricator via cfe-commits]

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:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Driver/ToolChains/Arch/RISCV.cpp
  clang/test/Driver/riscv-abi.c
  clang/test/Driver/riscv-arch.c
  llvm/include/llvm/Support/RISCVISAInfo.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/invalid-attribute.s

Index: llvm/test/MC/RISCV/invalid-attribute.s
===
--- llvm/test/MC/RISCV/invalid-attribute.s
+++ llvm/test/MC/RISCV/invalid-attribute.s
@@ -7,10 +7,10 @@
 # RUN: not llvm-mc %s -triple=riscv64 -filetype=asm 2>&1 | FileCheck %s
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: bad arch string foo
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,g}
 
 .attribute arch, "rv32i2p0_y2p0"
-# CHECK: [[@LINE-1]]:18: error: bad arch string y2p0
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p0_y2p0', invalid standard user-level extension 'y'
 
 .attribute stack_align, "16"
 # CHECK: [[@LINE-1]]:25: error: expected numeric constant
Index: llvm/test/MC/RISCV/attribute-with-insts.s
===
--- llvm/test/MC/RISCV/attribute-with-insts.s
+++ llvm/test/MC/RISCV/attribute-with-insts.s
@@ -10,7 +10,7 @@
 # RUN:   | llvm-objdump --triple=riscv64 -d -M no-aliases - \
 # RUN:   | FileCheck -check-prefix=CHECK-INST %s
 
-.attribute arch, "rv64i2p0_m2p0_a2p0_d2p0_c2p0"
+.attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 # CHECK-INST: lr.w t0, (t1)
 lr.w t0, (t1)
Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -9,9 +9,6 @@
 .attribute arch, "rv32i2"
 # CHECK: attribute  5, "rv32i2p0"
 
-.attribute arch, "rv32i2p"
-# CHECK: attribute  5, "rv32i2p0"
-
 .attribute arch, "rv32i2p0"
 # CHECK: attribute  5, "rv32i2p0"
 
@@ -33,14 +30,14 @@
 .attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
-.attribute arch, "rv32ima2p_fdc"
+.attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 .attribute arch, "rv32ib"
 # CHECK: attribute  5, "rv32i2p0_b0p93_zba0p93_zbb0p93_zbc0p93_zbe0p93_zbf0p93_zbm0p93_zbp0p93_zbr0p93_zbs0p93_zbt0p93"
 
 .attribute arch, "rv32iv"
-# CHECK: attribute  5, "rv32i2p0_v0p10"
+# CHECK: attribute  5, "rv32i2p0_v0p10_zvlsseg0p10"
 
 .attribute arch, "rv32izba"
 # CHECK: attribute  5, "rv32i2p0_zba0p93"
Index: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
===
--- llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
+++ llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
@@ -11,9 +11,11 @@
 //===--===//
 
 #include "RISCVTargetStreamer.h"
+#include "RISCVBaseInfo.h"
 #include "RISCVMCTargetDesc.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 
 using namespace llvm;
 
@@ -43,57 +45,13 @@
   else
 emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);
 
-  std::string Arch = "rv32";
-  if (STI.hasFeature(RISCV::Feature64Bit))
-Arch = "rv64";
-  if (STI.hasFeature(RISCV::FeatureRV32E))
-Arch += "e1p9";
-  else
-Arch += "i2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtM))
-Arch += "_m2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtA))
-Arch += "_a2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtF))
-Arch += "_f2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtD))
-Arch += "_d2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtC))
-Arch += "_c2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtB))
-Arch += "_b0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtV))
-Arch += "_v0p10";
-  if (STI.hasFeature(RISCV::FeatureExtZfh))
-Arch += "_zfh0p1";
-  if (STI.hasFeature(RISCV::FeatureExtZba))
-Arch += "_zba0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbb))
-Arch += "_zbb0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbc))
-Arch += "_zbc0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbe))
-Arch += "_zbe0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbf))
-Arch 

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

[Yueh-Ting Chen via Phabricator via cfe-commits]

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 , unsigned ,

For naming consistency with `parseArchString`, maybe rename `MArch` to `Arch`?


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

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

[Kito Cheng via Phabricator via cfe-commits]

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

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

https://reviews.llvm.org/D105168

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

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

[Kito Cheng via Phabricator via cfe-commits]

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:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Driver/ToolChains/Arch/RISCV.cpp
  clang/test/Driver/riscv-abi.c
  clang/test/Driver/riscv-arch.c
  llvm/include/llvm/Support/RISCVISAInfo.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/invalid-attribute.s

Index: llvm/test/MC/RISCV/invalid-attribute.s
===
--- llvm/test/MC/RISCV/invalid-attribute.s
+++ llvm/test/MC/RISCV/invalid-attribute.s
@@ -7,10 +7,10 @@
 # RUN: not llvm-mc %s -triple=riscv64 -filetype=asm 2>&1 | FileCheck %s
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: bad arch string foo
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,g}
 
 .attribute arch, "rv32i2p0_y2p0"
-# CHECK: [[@LINE-1]]:18: error: bad arch string y2p0
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p0_y2p0', invalid standard user-level extension 'y'
 
 .attribute stack_align, "16"
 # CHECK: [[@LINE-1]]:25: error: expected numeric constant
Index: llvm/test/MC/RISCV/attribute-with-insts.s
===
--- llvm/test/MC/RISCV/attribute-with-insts.s
+++ llvm/test/MC/RISCV/attribute-with-insts.s
@@ -10,7 +10,7 @@
 # RUN:   | llvm-objdump --triple=riscv64 -d -M no-aliases - \
 # RUN:   | FileCheck -check-prefix=CHECK-INST %s
 
-.attribute arch, "rv64i2p0_m2p0_a2p0_d2p0_c2p0"
+.attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 # CHECK-INST: lr.w t0, (t1)
 lr.w t0, (t1)
Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -9,9 +9,6 @@
 .attribute arch, "rv32i2"
 # CHECK: attribute  5, "rv32i2p0"
 
-.attribute arch, "rv32i2p"
-# CHECK: attribute  5, "rv32i2p0"
-
 .attribute arch, "rv32i2p0"
 # CHECK: attribute  5, "rv32i2p0"
 
@@ -33,14 +30,14 @@
 .attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
-.attribute arch, "rv32ima2p_fdc"
+.attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 .attribute arch, "rv32ib"
 # CHECK: attribute  5, "rv32i2p0_b0p93_zba0p93_zbb0p93_zbc0p93_zbe0p93_zbf0p93_zbm0p93_zbp0p93_zbr0p93_zbs0p93_zbt0p93"
 
 .attribute arch, "rv32iv"
-# CHECK: attribute  5, "rv32i2p0_v0p10"
+# CHECK: attribute  5, "rv32i2p0_v0p10_zvlsseg0p10"
 
 .attribute arch, "rv32izba"
 # CHECK: attribute  5, "rv32i2p0_zba0p93"
Index: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
===
--- llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
+++ llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
@@ -11,9 +11,11 @@
 //===--===//
 
 #include "RISCVTargetStreamer.h"
+#include "RISCVBaseInfo.h"
 #include "RISCVMCTargetDesc.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 
 using namespace llvm;
 
@@ -43,57 +45,13 @@
   else
 emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);
 
-  std::string Arch = "rv32";
-  if (STI.hasFeature(RISCV::Feature64Bit))
-Arch = "rv64";
-  if (STI.hasFeature(RISCV::FeatureRV32E))
-Arch += "e1p9";
-  else
-Arch += "i2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtM))
-Arch += "_m2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtA))
-Arch += "_a2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtF))
-Arch += "_f2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtD))
-Arch += "_d2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtC))
-Arch += "_c2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtB))
-Arch += "_b0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtV))
-Arch += "_v0p10";
-  if (STI.hasFeature(RISCV::FeatureExtZfh))
-Arch += "_zfh0p1";
-  if (STI.hasFeature(RISCV::FeatureExtZba))
-Arch += "_zba0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbb))
-Arch += "_zbb0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbc))
-Arch += "_zbc0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbe))
-Arch += "_zbe0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbf))
-Arch += 

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

[Alex Bradbury via Phabricator via cfe-commits]

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 
-I/home/asb/llvm-project/build/default/lib/Support 
-I/home/asb/llvm-project/llvm/lib/Support 
-I/home/asb/llvm-project/build/default/include 
-I/home/asb/llvm-project/llvm/include -fPIC -fvisibility-inlines-hidden 
-Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra 
-Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers 
-pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough 
-Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor 
-Wdelete-non-virtual-dtor -Wstring-conversion -Wmisleading-indentation 
-fdiagnostics-color -Werror=global-constructors -g -fPIC -gsplit-dwarf 
-std=c++14  -fno-exceptions -fno-rtti -MD -MT 
lib/Support/CMakeFiles/LLVMSupport.dir/RISCVISAInfo.cpp.o -MF 
lib/Support/CMakeFiles/LLVMSupport.dir/RISCVISAInfo.cpp.o.d -o 
lib/Support/CMakeFiles/LLVMSupport.dir/RISCVISAInfo.cpp.o -c 
/home/asb/llvm-project/llvm/lib/Support/RISCVISAInfo.cpp
  /home/asb/llvm-project/llvm/lib/Support/RISCVISAInfo.cpp:399:10: warning: 
moving a local object in a return statement prevents copy elision 
[-Wpessimizing-move]
return std::move(ISAInfo);
   ^
  /home/asb/llvm-project/llvm/lib/Support/RISCVISAInfo.cpp:399:10: note: remove 
std::move call here
return std::move(ISAInfo);
   ^~   ~
  /home/asb/llvm-project/llvm/lib/Support/RISCVISAInfo.cpp:478:12: error: call 
to deleted constructor of 'llvm::Error'
  return E;
 ^
  /home/asb/llvm-project/llvm/include/llvm/Support/Error.h:186:3: note: 'Error' 
has been explicitly marked deleted here
Error(const Error ) = delete;
^
  /home/asb/llvm-project/llvm/include/llvm/Support/Error.h:493:18: note: 
passing argument to parameter 'Err' here
Expected(Error Err)
   ^
  /home/asb/llvm-project/llvm/lib/Support/RISCVISAInfo.cpp:520:14: error: call 
to deleted constructor of 'llvm::Error'
return E;
   ^
  /home/asb/llvm-project/llvm/include/llvm/Support/Error.h:186:3: note: 'Error' 
has been explicitly marked deleted here
Error(const Error ) = delete;
^
  /home/asb/llvm-project/llvm/include/llvm/Support/Error.h:493:18: note: 
passing argument to parameter 'Err' here
Expected(Error Err)
   ^
  /home/asb/llvm-project/llvm/lib/Support/RISCVISAInfo.cpp:642:14: error: call 
to deleted constructor of 'llvm::Error'
return E;
   ^
  /home/asb/llvm-project/llvm/include/llvm/Support/Error.h:186:3: note: 'Error' 
has been explicitly marked deleted here
Error(const Error ) = delete;
^
  /home/asb/llvm-project/llvm/include/llvm/Support/Error.h:493:18: note: 
passing argument to parameter 'Err' here
Expected(Error Err)
   ^
  1 warning and 3 errors generated.
  [3/3711] Building CXX object 
tools/llvm-config/CMakeFiles/llvm-config.dir/llvm-config.cpp.o
  ninja: build stopped: subcommand failed.


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

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

[Kito Cheng via Phabricator via cfe-commits]

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

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

[Kito Cheng via Phabricator via cfe-commits]

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`
  - `SupportedExperimentalExtensionInfos` -> `SupportedExperimentalExtensions`
- Change return type of parseArchString to 
`llvm::Expected>`
- Address @jrtc27's comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Driver/ToolChains/Arch/RISCV.cpp
  clang/test/Driver/riscv-abi.c
  clang/test/Driver/riscv-arch.c
  llvm/include/llvm/Support/RISCVISAInfo.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/invalid-attribute.s

Index: llvm/test/MC/RISCV/invalid-attribute.s
===
--- llvm/test/MC/RISCV/invalid-attribute.s
+++ llvm/test/MC/RISCV/invalid-attribute.s
@@ -7,10 +7,10 @@
 # RUN: not llvm-mc %s -triple=riscv64 -filetype=asm 2>&1 | FileCheck %s
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: bad arch string foo
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,g}
 
 .attribute arch, "rv32i2p0_y2p0"
-# CHECK: [[@LINE-1]]:18: error: bad arch string y2p0
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p0_y2p0', invalid standard user-level extension 'y'
 
 .attribute stack_align, "16"
 # CHECK: [[@LINE-1]]:25: error: expected numeric constant
Index: llvm/test/MC/RISCV/attribute-with-insts.s
===
--- llvm/test/MC/RISCV/attribute-with-insts.s
+++ llvm/test/MC/RISCV/attribute-with-insts.s
@@ -10,7 +10,7 @@
 # RUN:   | llvm-objdump --triple=riscv64 -d -M no-aliases - \
 # RUN:   | FileCheck -check-prefix=CHECK-INST %s
 
-.attribute arch, "rv64i2p0_m2p0_a2p0_d2p0_c2p0"
+.attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 # CHECK-INST: lr.w t0, (t1)
 lr.w t0, (t1)
Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -9,9 +9,6 @@
 .attribute arch, "rv32i2"
 # CHECK: attribute  5, "rv32i2p0"
 
-.attribute arch, "rv32i2p"
-# CHECK: attribute  5, "rv32i2p0"
-
 .attribute arch, "rv32i2p0"
 # CHECK: attribute  5, "rv32i2p0"
 
@@ -33,14 +30,14 @@
 .attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
-.attribute arch, "rv32ima2p_fdc"
+.attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 .attribute arch, "rv32ib"
 # CHECK: attribute  5, "rv32i2p0_b0p93_zba0p93_zbb0p93_zbc0p93_zbe0p93_zbf0p93_zbm0p93_zbp0p93_zbr0p93_zbs0p93_zbt0p93"
 
 .attribute arch, "rv32iv"
-# CHECK: attribute  5, "rv32i2p0_v0p10"
+# CHECK: attribute  5, "rv32i2p0_v0p10_zvlsseg0p10"
 
 .attribute arch, "rv32izba"
 # CHECK: attribute  5, "rv32i2p0_zba0p93"
Index: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
===
--- llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
+++ llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
@@ -11,9 +11,11 @@
 //===--===//
 
 #include "RISCVTargetStreamer.h"
+#include "RISCVBaseInfo.h"
 #include "RISCVMCTargetDesc.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 
 using namespace llvm;
 
@@ -43,57 +45,13 @@
   else
 emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);
 
-  std::string Arch = "rv32";
-  if (STI.hasFeature(RISCV::Feature64Bit))
-Arch = "rv64";
-  if (STI.hasFeature(RISCV::FeatureRV32E))
-Arch += "e1p9";
-  else
-Arch += "i2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtM))
-Arch += "_m2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtA))
-Arch += "_a2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtF))
-Arch += "_f2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtD))
-Arch += "_d2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtC))
-Arch += "_c2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtB))
-Arch += "_b0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtV))
-Arch += "_v0p10";
-  if (STI.hasFeature(RISCV::FeatureExtZfh))
-Arch += "_zfh0p1";
-  if (STI.hasFeature(RISCV::FeatureExtZba))
-

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

[Kito Cheng via Phabricator via cfe-commits]

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", RISCVExtensionVersion{2, 0}},

jrtc27 wrote:
> I'd keep these all one per line
Although it's format by clang-format, but I like one per line too, let me 
manually update that.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:107
+struct FindByName {
+  FindByName(StringRef Ext) : Ext(Ext){};
+  StringRef Ext;

jrtc27 wrote:
> 
It's format by clang-format, but either is fine to me :P



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:378
+llvm::find_if(ExtensionInfos, FindByName(ExtName));
+if (ExtensionInfoIterator == ExtensionInfos.end())
+  continue;

jrtc27 wrote:
> No error?..
Not all features is related to extension, like `relax` or `save-restore`, so 
here we just ignore, but add few comment here seem better.


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

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

[Jessica Clarke via Phabricator via cfe-commits]

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,

Use llvm::Expected<...> as the return value to avoid a separate error out param



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:40
+
+static const RISCVSupportedExtensionInfo SupportedExtensionInfos[] = {
+{"i", RISCVExtensionVersion{2, 0}}, {"e", RISCVExtensionVersion{1, 9}},

Is the "Infos" suffix on the variable name really needed?



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", RISCVExtensionVersion{2, 0}},

I'd keep these all one per line



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:107
+struct FindByName {
+  FindByName(StringRef Ext) : Ext(Ext){};
+  StringRef Ext;





Comment at: llvm/lib/Support/RISCVISAInfo.cpp:378
+llvm::find_if(ExtensionInfos, FindByName(ExtName));
+if (ExtensionInfoIterator == ExtensionInfos.end())
+  continue;

No error?..


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

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

[Kito Cheng via Phabricator via cfe-commits]

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 will construct an empty map.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:194
+  switch (ExtClass) {
+  case 's':
+HighOrder = 0;

jrtc27 wrote:
> I imagine this needs to change to the new Sm-/Ss- scheme, but I don't know 
> which way round those are intended to go
I would prefer submit another patch to make this parser up to date, and keep 
this patch as a refactor patch as possible:

e.g.:
- Full implication info, e.g. `d` implied `f`, `f` implied `zicsr`...
- Prefix `zxm`.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:197
+break;
+  case 'h':
+HighOrder = 1;

jrtc27 wrote:
> Do we know if this is still the case? Ss- is being used for S-mode extensions 
> and Sm- for M-mode extensions, so I'd expect Sh- (or maybe just Ss-) for 
> HS-mode extensions, not H-.
Like above reply, prefer another patch to fix that.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:225-229
+  if (LHSLen == 1 && RHSLen != 1)
+return true;
+
+  if (LHSLen != 1 && RHSLen == 1)
+return false;

jrtc27 wrote:
> Don't know if this is better or not, but this is the more compact way to 
> write it
It's more compact but it's hard to read the rule to me, so I would prefer keep 
that as it is


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

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

[Kito Cheng via Phabricator via cfe-commits]

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.
- Address @jrtc27's comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Driver/ToolChains/Arch/RISCV.cpp
  clang/test/Driver/riscv-abi.c
  clang/test/Driver/riscv-arch.c
  llvm/include/llvm/Support/RISCVISAInfo.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/invalid-attribute.s

Index: llvm/test/MC/RISCV/invalid-attribute.s
===
--- llvm/test/MC/RISCV/invalid-attribute.s
+++ llvm/test/MC/RISCV/invalid-attribute.s
@@ -7,10 +7,10 @@
 # RUN: not llvm-mc %s -triple=riscv64 -filetype=asm 2>&1 | FileCheck %s
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: bad arch string foo
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,g}
 
 .attribute arch, "rv32i2p0_y2p0"
-# CHECK: [[@LINE-1]]:18: error: bad arch string y2p0
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p0_y2p0', invalid standard user-level extension 'y'
 
 .attribute stack_align, "16"
 # CHECK: [[@LINE-1]]:25: error: expected numeric constant
Index: llvm/test/MC/RISCV/attribute-with-insts.s
===
--- llvm/test/MC/RISCV/attribute-with-insts.s
+++ llvm/test/MC/RISCV/attribute-with-insts.s
@@ -10,7 +10,7 @@
 # RUN:   | llvm-objdump --triple=riscv64 -d -M no-aliases - \
 # RUN:   | FileCheck -check-prefix=CHECK-INST %s
 
-.attribute arch, "rv64i2p0_m2p0_a2p0_d2p0_c2p0"
+.attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 # CHECK-INST: lr.w t0, (t1)
 lr.w t0, (t1)
Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -9,9 +9,6 @@
 .attribute arch, "rv32i2"
 # CHECK: attribute  5, "rv32i2p0"
 
-.attribute arch, "rv32i2p"
-# CHECK: attribute  5, "rv32i2p0"
-
 .attribute arch, "rv32i2p0"
 # CHECK: attribute  5, "rv32i2p0"
 
@@ -33,14 +30,14 @@
 .attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
-.attribute arch, "rv32ima2p_fdc"
+.attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 .attribute arch, "rv32ib"
 # CHECK: attribute  5, "rv32i2p0_b0p93_zba0p93_zbb0p93_zbc0p93_zbe0p93_zbf0p93_zbm0p93_zbp0p93_zbr0p93_zbs0p93_zbt0p93"
 
 .attribute arch, "rv32iv"
-# CHECK: attribute  5, "rv32i2p0_v0p10"
+# CHECK: attribute  5, "rv32i2p0_v0p10_zvlsseg0p10"
 
 .attribute arch, "rv32izba"
 # CHECK: attribute  5, "rv32i2p0_zba0p93"
Index: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
===
--- llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
+++ llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
@@ -11,9 +11,11 @@
 //===--===//
 
 #include "RISCVTargetStreamer.h"
+#include "RISCVBaseInfo.h"
 #include "RISCVMCTargetDesc.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 
 using namespace llvm;
 
@@ -43,57 +45,13 @@
   else
 emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);
 
-  std::string Arch = "rv32";
-  if (STI.hasFeature(RISCV::Feature64Bit))
-Arch = "rv64";
-  if (STI.hasFeature(RISCV::FeatureRV32E))
-Arch += "e1p9";
-  else
-Arch += "i2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtM))
-Arch += "_m2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtA))
-Arch += "_a2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtF))
-Arch += "_f2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtD))
-Arch += "_d2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtC))
-Arch += "_c2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtB))
-Arch += "_b0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtV))
-Arch += "_v0p10";
-  if (STI.hasFeature(RISCV::FeatureExtZfh))
-Arch += "_zfh0p1";
-  if (STI.hasFeature(RISCV::FeatureExtZba))
-Arch += "_zba0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbb))
-Arch += "_zbb0p93";
-  if 

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

[Jessica Clarke via Phabricator via cfe-commits]

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: llvm/include/llvm/Support/RISCVISAInfo.h:47-52
+  /// Parse RISCV ISA info from arch string.
+  Error parseArchString(StringRef Arch, bool EnableExperimentalExtension,
+bool ExperimentalExtensionVersionCheck = true);
+
+  /// Parse RISCV ISA info from feature vector.
+  void parseFeatures(unsigned XLen, const std::vector );

I wonder, should these be constructors instead? `RISCVISAInfo ISAInfo; 
ISAInfo.parse` doesn't feel very idiomatic C++



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:178
+// order, but after all known standard extensions.
+Rank = AllStdExts.size() + (Ext - 'a');
+  else

Why not just return these directly? Don't need a variable.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:194
+  switch (ExtClass) {
+  case 's':
+HighOrder = 0;

I imagine this needs to change to the new Sm-/Ss- scheme, but I don't know 
which way round those are intended to go



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:197
+break;
+  case 'h':
+HighOrder = 1;

Do we know if this is still the case? Ss- is being used for S-mode extensions 
and Sm- for M-mode extensions, so I'd expect Sh- (or maybe just Ss-) for 
HS-mode extensions, not H-.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:207
+  default:
+assert(false && "Unknown prefix for multi-char extension");
+return -1;

llvm_unreachable



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:214
+  if (ExtClass == 'z')
+LowOrder = singleLetterExtensionRank(ExtName[1]);
+

Why not put this in its switch case?



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:225-229
+  if (LHSLen == 1 && RHSLen != 1)
+return true;
+
+  if (LHSLen != 1 && RHSLen == 1)
+return false;

Don't know if this is better or not, but this is the more compact way to write 
it



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:231
+  size_t RHSLen = RHS.length();
+  int LHSRank, RHSRank;
+  if (LHSLen == 1 && RHSLen != 1)

frasercrmck wrote:
> Not sure why these need to be declared up here.
Yeah, declare the variables at their assignments. Besides, you don't even need 
variables for the single-letter case, just compare the return values of the 
functions directly.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:40
+
+static const StringRef AllStdExts = "mafdqlcbjtpvn";
+

kito-cheng wrote:
> jrtc27 wrote:
> > craig.topper wrote:
> > > Make this `static constexpr StringLiteral AllStdExts = "mafdqlcbjtpvn";`
> > I can't help but feel this is really an array of chars, not a string. We 
> > don't even need the trailing NUL, though double quote syntax is simpler 
> > than curly braces and a bunch of single-quote chars just to save a byte.
> Yeah, it's actually just an array of chars, but we have use find function 
> from StringRef :p
Still not a StringLiteral



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:43
+static const RISCVSupportedExtensionInfo SupportedExtensionInfos[] = {
+{"i", RISCVExtensionVersion{2, 0}, /* Experimental */ false},
+{"e", RISCVExtensionVersion{1, 9}, /* Experimental */ false},

jrtc27 wrote:
> but that's the default so I'd omit it for anything other than the cases where 
> it's true
I do think it would be better to not list Experimental for the non-experimental 
ones


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

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

[Kito Cheng via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D105168

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Driver/ToolChains/Arch/RISCV.cpp
  clang/test/Driver/riscv-abi.c
  clang/test/Driver/riscv-arch.c
  llvm/include/llvm/Support/RISCVISAInfo.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/invalid-attribute.s

Index: llvm/test/MC/RISCV/invalid-attribute.s
===
--- llvm/test/MC/RISCV/invalid-attribute.s
+++ llvm/test/MC/RISCV/invalid-attribute.s
@@ -7,10 +7,10 @@
 # RUN: not llvm-mc %s -triple=riscv64 -filetype=asm 2>&1 | FileCheck %s
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: bad arch string foo
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,g}
 
 .attribute arch, "rv32i2p0_y2p0"
-# CHECK: [[@LINE-1]]:18: error: bad arch string y2p0
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p0_y2p0', invalid standard user-level extension 'y'
 
 .attribute stack_align, "16"
 # CHECK: [[@LINE-1]]:25: error: expected numeric constant
Index: llvm/test/MC/RISCV/attribute-with-insts.s
===
--- llvm/test/MC/RISCV/attribute-with-insts.s
+++ llvm/test/MC/RISCV/attribute-with-insts.s
@@ -10,7 +10,7 @@
 # RUN:   | llvm-objdump --triple=riscv64 -d -M no-aliases - \
 # RUN:   | FileCheck -check-prefix=CHECK-INST %s
 
-.attribute arch, "rv64i2p0_m2p0_a2p0_d2p0_c2p0"
+.attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 # CHECK-INST: lr.w t0, (t1)
 lr.w t0, (t1)
Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -9,9 +9,6 @@
 .attribute arch, "rv32i2"
 # CHECK: attribute  5, "rv32i2p0"
 
-.attribute arch, "rv32i2p"
-# CHECK: attribute  5, "rv32i2p0"
-
 .attribute arch, "rv32i2p0"
 # CHECK: attribute  5, "rv32i2p0"
 
@@ -33,14 +30,14 @@
 .attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
-.attribute arch, "rv32ima2p_fdc"
+.attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 .attribute arch, "rv32ib"
 # CHECK: attribute  5, "rv32i2p0_b0p93_zba0p93_zbb0p93_zbc0p93_zbe0p93_zbf0p93_zbm0p93_zbp0p93_zbr0p93_zbs0p93_zbt0p93"
 
 .attribute arch, "rv32iv"
-# CHECK: attribute  5, "rv32i2p0_v0p10"
+# CHECK: attribute  5, "rv32i2p0_v0p10_zvlsseg0p10"
 
 .attribute arch, "rv32izba"
 # CHECK: attribute  5, "rv32i2p0_zba0p93"
Index: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
===
--- llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
+++ llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
@@ -11,9 +11,11 @@
 //===--===//
 
 #include "RISCVTargetStreamer.h"
+#include "RISCVBaseInfo.h"
 #include "RISCVMCTargetDesc.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 
 using namespace llvm;
 
@@ -43,57 +45,14 @@
   else
 emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);
 
-  std::string Arch = "rv32";
-  if (STI.hasFeature(RISCV::Feature64Bit))
-Arch = "rv64";
-  if (STI.hasFeature(RISCV::FeatureRV32E))
-Arch += "e1p9";
-  else
-Arch += "i2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtM))
-Arch += "_m2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtA))
-Arch += "_a2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtF))
-Arch += "_f2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtD))
-Arch += "_d2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtC))
-Arch += "_c2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtB))
-Arch += "_b0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtV))
-Arch += "_v0p10";
-  if (STI.hasFeature(RISCV::FeatureExtZfh))
-Arch += "_zfh0p1";
-  if (STI.hasFeature(RISCV::FeatureExtZba))
-Arch += "_zba0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbb))
-Arch += "_zbb0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbc))
-Arch += "_zbc0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbe))
-Arch += "_zbe0p93";
-  if 

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

[Fraser Cormack via Phabricator via cfe-commits]

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 simpler.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:147
+unsigned MinorVersion) {
+  for (auto const  :
+   filterSupportedExtensionInfosByName(Ext)) {

This also looks like a `find_if`



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:231
+  size_t RHSLen = RHS.length();
+  int LHSRank, RHSRank;
+  if (LHSLen == 1 && RHSLen != 1)

Not sure why these need to be declared up here.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:310
+errc::invalid_argument,
+"Failed to parsing major version number for extension '" + Ext + "'");
+

`Failed to parse ...`



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:315
+errc::invalid_argument,
+"Failed to parsing minor version number for extension '" + Ext + "'");
+

`Failed to parse`



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:359
+}
+// return true;
+return Error::success();

Commented-out code



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:414
+  // RISC-V ISA strings must be lowercase.
+  if (llvm::any_of(Arch, [](char C) { return isupper(C); })) {
+return createStringError(errc::invalid_argument,

I think you can just have `if (llvm::any_of(Arch, isupper))` here.



Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2036
+for (auto Feature : RISCVFeatureKV) {
+  if (llvm::RISCVISAInfo::isSupportedExtensionFeature(Feature.Key)) {
+clearFeatureBits(Feature.Value, Feature.Key);

Don't need `{}` here



Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2055
+
+for (auto Feature : RISCVFeatureKV) {
+  if (ISAInfo.hasExtension(Feature.Key)) {

Don't need `{}` here either


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

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

[Kito Cheng via Phabricator via cfe-commits]

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:
> These comments aren't helpful. If you want to write full doxygen then you 
> can, but a one-line comment that won't appear in documentation and is obvious 
> from the function name and signature is just clutter.
Rename RISCVISAInfo::parse to 
RISCVISAInfo::parseFeatures/RISCVISAInfo::parseArchString, I guess it should be 
more clear.


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

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

[Kito Cheng via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D105168/new/

https://reviews.llvm.org/D105168

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Driver/ToolChains/Arch/RISCV.cpp
  clang/test/Driver/riscv-abi.c
  clang/test/Driver/riscv-arch.c
  llvm/include/llvm/Support/RISCVISAInfo.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/invalid-attribute.s

Index: llvm/test/MC/RISCV/invalid-attribute.s
===
--- llvm/test/MC/RISCV/invalid-attribute.s
+++ llvm/test/MC/RISCV/invalid-attribute.s
@@ -7,10 +7,10 @@
 # RUN: not llvm-mc %s -triple=riscv64 -filetype=asm 2>&1 | FileCheck %s
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: bad arch string foo
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,g}
 
 .attribute arch, "rv32i2p0_y2p0"
-# CHECK: [[@LINE-1]]:18: error: bad arch string y2p0
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p0_y2p0', invalid standard user-level extension 'y'
 
 .attribute stack_align, "16"
 # CHECK: [[@LINE-1]]:25: error: expected numeric constant
Index: llvm/test/MC/RISCV/attribute-with-insts.s
===
--- llvm/test/MC/RISCV/attribute-with-insts.s
+++ llvm/test/MC/RISCV/attribute-with-insts.s
@@ -10,7 +10,7 @@
 # RUN:   | llvm-objdump --triple=riscv64 -d -M no-aliases - \
 # RUN:   | FileCheck -check-prefix=CHECK-INST %s
 
-.attribute arch, "rv64i2p0_m2p0_a2p0_d2p0_c2p0"
+.attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 # CHECK-INST: lr.w t0, (t1)
 lr.w t0, (t1)
Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -9,9 +9,6 @@
 .attribute arch, "rv32i2"
 # CHECK: attribute  5, "rv32i2p0"
 
-.attribute arch, "rv32i2p"
-# CHECK: attribute  5, "rv32i2p0"
-
 .attribute arch, "rv32i2p0"
 # CHECK: attribute  5, "rv32i2p0"
 
@@ -33,14 +30,14 @@
 .attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
-.attribute arch, "rv32ima2p_fdc"
+.attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 .attribute arch, "rv32ib"
 # CHECK: attribute  5, "rv32i2p0_b0p93_zba0p93_zbb0p93_zbc0p93_zbe0p93_zbf0p93_zbm0p93_zbp0p93_zbr0p93_zbs0p93_zbt0p93"
 
 .attribute arch, "rv32iv"
-# CHECK: attribute  5, "rv32i2p0_v0p10"
+# CHECK: attribute  5, "rv32i2p0_v0p10_zvlsseg0p10"
 
 .attribute arch, "rv32izba"
 # CHECK: attribute  5, "rv32i2p0_zba0p93"
Index: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
===
--- llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
+++ llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
@@ -11,9 +11,11 @@
 //===--===//
 
 #include "RISCVTargetStreamer.h"
+#include "RISCVBaseInfo.h"
 #include "RISCVMCTargetDesc.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 
 using namespace llvm;
 
@@ -43,57 +45,14 @@
   else
 emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);
 
-  std::string Arch = "rv32";
-  if (STI.hasFeature(RISCV::Feature64Bit))
-Arch = "rv64";
-  if (STI.hasFeature(RISCV::FeatureRV32E))
-Arch += "e1p9";
-  else
-Arch += "i2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtM))
-Arch += "_m2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtA))
-Arch += "_a2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtF))
-Arch += "_f2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtD))
-Arch += "_d2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtC))
-Arch += "_c2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtB))
-Arch += "_b0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtV))
-Arch += "_v0p10";
-  if (STI.hasFeature(RISCV::FeatureExtZfh))
-Arch += "_zfh0p1";
-  if (STI.hasFeature(RISCV::FeatureExtZba))
-Arch += "_zba0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbb))
-Arch += "_zbb0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbc))
-Arch += "_zbc0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbe))

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

[Kito Cheng via Phabricator via cfe-commits]

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: invalid arch name 'rv32e',
-// RV32E: standard user-level extension 'e'

jrtc27 wrote:
> Hm, given we don't currently support RV32E properly this should probably be 
> an error still, but could be a "nicer" error than a generic "invalid arch 
> name" (which is technically wrong)
We support E-extension on MC-layer...but not support `ilp32e`.

This patch unify the ISA stuffs, so either I need to remove that from MC, or I 
need to made rv32e supported on driver...

https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp#L50
https://github.com/llvm/llvm-project/blob/main/llvm/test/MC/RISCV/rv32e-valid.s
https://github.com/llvm/llvm-project/blob/main/llvm/test/MC/RISCV/rv32e-invalid.s



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:40
+
+static const StringRef AllStdExts = "mafdqlcbjtpvn";
+

jrtc27 wrote:
> craig.topper wrote:
> > Make this `static constexpr StringLiteral AllStdExts = "mafdqlcbjtpvn";`
> I can't help but feel this is really an array of chars, not a string. We 
> don't even need the trailing NUL, though double quote syntax is simpler than 
> curly braces and a bunch of single-quote chars just to save a byte.
Yeah, it's actually just an array of chars, but we have use find function from 
StringRef :p



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:135
+  if (CheckExperimental)
+IsExperimental = stripExperimentalPrefix(Ext);
+

jrtc27 wrote:
> *Extensions* don't have an experimental- prefix, *internal target features* 
> do, so something strange is going on here. This feels like we're confusing 
> several things:
> 1. Whether or not Ext is a feature or an extension
> 2. Whether or not Ext is experimental
> 3. Whether we want to look at experimental extensions
> Some of those are somewhat necessarily interwoven, but the naming does not 
> currently accurately reflect what these things mean, and I would argue we 
> should be very explicit and keep features and extensions separate, never 
> using the same thing to represent both.
Good point, CheckExperimental is kind of ambiguous,  new function: 
isSupportedExtensionFeature added.


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

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

[Kito Cheng via Phabricator via cfe-commits]

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Driver/ToolChains/Arch/RISCV.cpp
  clang/test/Driver/riscv-abi.c
  clang/test/Driver/riscv-arch.c
  llvm/include/llvm/Support/RISCVISAInfo.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/invalid-attribute.s

Index: llvm/test/MC/RISCV/invalid-attribute.s
===
--- llvm/test/MC/RISCV/invalid-attribute.s
+++ llvm/test/MC/RISCV/invalid-attribute.s
@@ -7,10 +7,10 @@
 # RUN: not llvm-mc %s -triple=riscv64 -filetype=asm 2>&1 | FileCheck %s
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: bad arch string foo
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,g}
 
 .attribute arch, "rv32i2p0_y2p0"
-# CHECK: [[@LINE-1]]:18: error: bad arch string y2p0
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p0_y2p0', invalid standard user-level extension 'y'
 
 .attribute stack_align, "16"
 # CHECK: [[@LINE-1]]:25: error: expected numeric constant
Index: llvm/test/MC/RISCV/attribute-with-insts.s
===
--- llvm/test/MC/RISCV/attribute-with-insts.s
+++ llvm/test/MC/RISCV/attribute-with-insts.s
@@ -10,7 +10,7 @@
 # RUN:   | llvm-objdump --triple=riscv64 -d -M no-aliases - \
 # RUN:   | FileCheck -check-prefix=CHECK-INST %s
 
-.attribute arch, "rv64i2p0_m2p0_a2p0_d2p0_c2p0"
+.attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 # CHECK-INST: lr.w t0, (t1)
 lr.w t0, (t1)
Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -9,9 +9,6 @@
 .attribute arch, "rv32i2"
 # CHECK: attribute  5, "rv32i2p0"
 
-.attribute arch, "rv32i2p"
-# CHECK: attribute  5, "rv32i2p0"
-
 .attribute arch, "rv32i2p0"
 # CHECK: attribute  5, "rv32i2p0"
 
@@ -33,14 +30,14 @@
 .attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
-.attribute arch, "rv32ima2p_fdc"
+.attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 .attribute arch, "rv32ib"
 # CHECK: attribute  5, "rv32i2p0_b0p93_zba0p93_zbb0p93_zbc0p93_zbe0p93_zbf0p93_zbm0p93_zbp0p93_zbr0p93_zbs0p93_zbt0p93"
 
 .attribute arch, "rv32iv"
-# CHECK: attribute  5, "rv32i2p0_v0p10"
+# CHECK: attribute  5, "rv32i2p0_v0p10_zvlsseg0p10"
 
 .attribute arch, "rv32izba"
 # CHECK: attribute  5, "rv32i2p0_zba0p93"
Index: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
===
--- llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
+++ llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
@@ -11,9 +11,11 @@
 //===--===//
 
 #include "RISCVTargetStreamer.h"
+#include "RISCVBaseInfo.h"
 #include "RISCVMCTargetDesc.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 
 using namespace llvm;
 
@@ -43,57 +45,14 @@
   else
 emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);
 
-  std::string Arch = "rv32";
-  if (STI.hasFeature(RISCV::Feature64Bit))
-Arch = "rv64";
-  if (STI.hasFeature(RISCV::FeatureRV32E))
-Arch += "e1p9";
-  else
-Arch += "i2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtM))
-Arch += "_m2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtA))
-Arch += "_a2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtF))
-Arch += "_f2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtD))
-Arch += "_d2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtC))
-Arch += "_c2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtB))
-Arch += "_b0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtV))
-Arch += "_v0p10";
-  if (STI.hasFeature(RISCV::FeatureExtZfh))
-Arch += "_zfh0p1";
-  if (STI.hasFeature(RISCV::FeatureExtZba))
-Arch += "_zba0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbb))
-Arch += "_zbb0p93";
-  if 

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

[Jessica Clarke via Phabricator via cfe-commits]

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:   | FileCheck -check-prefix=CHECK-LP64D %s

khchen wrote:
> Why do we need to modify here?
rv64d is not a valid arch string. rv64id is though; would that work here? These 
tests are just currently testing behaviour that _should_ have been an error... 
(see also rv64 f above)



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: invalid arch name 'rv32e',
-// RV32E: standard user-level extension 'e'

Hm, given we don't currently support RV32E properly this should probably be an 
error still, but could be a "nicer" error than a generic "invalid arch name" 
(which is technically wrong)



Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:45
+
+  // Parse RISCV ISA info from arch string.
+  Error parse(StringRef Arch, bool EnableExperimentalExtension,

These comments aren't helpful. If you want to write full doxygen then you can, 
but a one-line comment that won't appear in documentation and is obvious from 
the function name and signature is just clutter.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:24
+namespace {
+/// Represents the major and version number components of a RISC-V extension
+struct RISCVExtensionVersion {

This seems unnecessary



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:32
+  const char *Name;
+  /// Supported version.
+  RISCVExtensionVersion Version;

Ditto for these



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:40
+
+static const StringRef AllStdExts = "mafdqlcbjtpvn";
+

craig.topper wrote:
> Make this `static constexpr StringLiteral AllStdExts = "mafdqlcbjtpvn";`
I can't help but feel this is really an array of chars, not a string. We don't 
even need the trailing NUL, though double quote syntax is simpler than curly 
braces and a bunch of single-quote chars just to save a byte.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:43
+static const RISCVSupportedExtensionInfo SupportedExtensionInfos[] = {
+{"i", RISCVExtensionVersion{2, 0}, /* Experimental */ false},
+{"e", RISCVExtensionVersion{1, 9}, /* Experimental */ false},

but that's the default so I'd omit it for anything other than the cases where 
it's true



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:71-72
+
+// Remove 'experimental-' if it's prefix of string,
+// return true if did the strip.
+static bool stripExperimentalPrefix(StringRef ) {

Comment isn't useful, it's a trivial wrapper



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:77
+
+RISCVISAInfo::RISCVISAInfo() : XLen(0), FLen(0) {}
+

Can this not go in the header as a trivial constructor?



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:112
+
+// Helper function for filtering SupportedExtensionInfos by name.
+static auto filterSupportedExtensionInfosByName(StringRef ExtName) {

Obvious comment



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:135
+  if (CheckExperimental)
+IsExperimental = stripExperimentalPrefix(Ext);
+

*Extensions* don't have an experimental- prefix, *internal target features* do, 
so something strange is going on here. This feels like we're confusing several 
things:
1. Whether or not Ext is a feature or an extension
2. Whether or not Ext is experimental
3. Whether we want to look at experimental extensions
Some of those are somewhat necessarily interwoven, but the naming does not 
currently accurately reflect what these things mean, and I would argue we 
should be very explicit and keep features and extensions separate, never using 
the same thing to represent both.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:193
+// order, but after all known standard extensions.
+Rank = AllStdExts.size() + (Ext - 'a');
+  else

This is wrong as it doesn't include the +2 to account for I and E, though does 
happen to work because A and B are both known single-letter standard extensions 
so you can't actually get it to overlap (but were AllStdExts to omit one of 
them you would be able to).

IMO though it would be cleaner to use negative numbers for I and E, as they're 
special, and that means you can just start the counting at 0 for all the 
others, not needing the +2 anywhere.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:235-236
+
+// Compare function for extension.
+// Only compare the extension name, 

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

[Craig Topper via Phabricator via cfe-commits]

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: llvm/lib/Support/RISCVISAInfo.cpp:396
+bool Add = ExtName[0] == '+';
+ExtName = ExtName.drop_front(1); // Drop '+'
+Experimental = stripExperimentalPrefix(ExtName);

This comment should be "Drop '+' or '-'" I think?



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:544
+  // Currently LLVM supports only "mafdcbv".
+  {
+return createStringError(

This is an unusual placement of curly braces. Put the comment inside?


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

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

[Kito Cheng via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D105168

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Driver/ToolChains/Arch/RISCV.cpp
  clang/test/Driver/riscv-abi.c
  clang/test/Driver/riscv-arch.c
  llvm/include/llvm/Support/RISCVISAInfo.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/invalid-attribute.s

Index: llvm/test/MC/RISCV/invalid-attribute.s
===
--- llvm/test/MC/RISCV/invalid-attribute.s
+++ llvm/test/MC/RISCV/invalid-attribute.s
@@ -7,10 +7,10 @@
 # RUN: not llvm-mc %s -triple=riscv64 -filetype=asm 2>&1 | FileCheck %s
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: bad arch string foo
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,g}
 
 .attribute arch, "rv32i2p0_y2p0"
-# CHECK: [[@LINE-1]]:18: error: bad arch string y2p0
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p0_y2p0', invalid standard user-level extension 'y'
 
 .attribute stack_align, "16"
 # CHECK: [[@LINE-1]]:25: error: expected numeric constant
Index: llvm/test/MC/RISCV/attribute-with-insts.s
===
--- llvm/test/MC/RISCV/attribute-with-insts.s
+++ llvm/test/MC/RISCV/attribute-with-insts.s
@@ -10,7 +10,7 @@
 # RUN:   | llvm-objdump --triple=riscv64 -d -M no-aliases - \
 # RUN:   | FileCheck -check-prefix=CHECK-INST %s
 
-.attribute arch, "rv64i2p0_m2p0_a2p0_d2p0_c2p0"
+.attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 # CHECK-INST: lr.w t0, (t1)
 lr.w t0, (t1)
Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -9,9 +9,6 @@
 .attribute arch, "rv32i2"
 # CHECK: attribute  5, "rv32i2p0"
 
-.attribute arch, "rv32i2p"
-# CHECK: attribute  5, "rv32i2p0"
-
 .attribute arch, "rv32i2p0"
 # CHECK: attribute  5, "rv32i2p0"
 
@@ -33,14 +30,14 @@
 .attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
-.attribute arch, "rv32ima2p_fdc"
+.attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 .attribute arch, "rv32ib"
 # CHECK: attribute  5, "rv32i2p0_b0p93_zba0p93_zbb0p93_zbc0p93_zbe0p93_zbf0p93_zbm0p93_zbp0p93_zbr0p93_zbs0p93_zbt0p93"
 
 .attribute arch, "rv32iv"
-# CHECK: attribute  5, "rv32i2p0_v0p10"
+# CHECK: attribute  5, "rv32i2p0_v0p10_zvlsseg0p10"
 
 .attribute arch, "rv32izba"
 # CHECK: attribute  5, "rv32i2p0_zba0p93"
Index: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
===
--- llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
+++ llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
@@ -11,9 +11,11 @@
 //===--===//
 
 #include "RISCVTargetStreamer.h"
+#include "RISCVBaseInfo.h"
 #include "RISCVMCTargetDesc.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 
 using namespace llvm;
 
@@ -43,57 +45,14 @@
   else
 emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);
 
-  std::string Arch = "rv32";
-  if (STI.hasFeature(RISCV::Feature64Bit))
-Arch = "rv64";
-  if (STI.hasFeature(RISCV::FeatureRV32E))
-Arch += "e1p9";
-  else
-Arch += "i2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtM))
-Arch += "_m2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtA))
-Arch += "_a2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtF))
-Arch += "_f2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtD))
-Arch += "_d2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtC))
-Arch += "_c2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtB))
-Arch += "_b0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtV))
-Arch += "_v0p10";
-  if (STI.hasFeature(RISCV::FeatureExtZfh))
-Arch += "_zfh0p1";
-  if (STI.hasFeature(RISCV::FeatureExtZba))
-Arch += "_zba0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbb))
-Arch += "_zbb0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbc))
-Arch += "_zbc0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbe))
-Arch += "_zbe0p93";
-  if 

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

[Kito Cheng via Phabricator via cfe-commits]

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 more than nit :p


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

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

[Zakk Chen via Phabricator via cfe-commits]

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.



Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:57
+}
+
 namespace {

nit: please fix this tidy warning


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

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

[Kito Cheng via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D105168

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Driver/ToolChains/Arch/RISCV.cpp
  clang/test/Driver/riscv-abi.c
  clang/test/Driver/riscv-arch.c
  llvm/include/llvm/Support/RISCVISAInfo.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/invalid-attribute.s

Index: llvm/test/MC/RISCV/invalid-attribute.s
===
--- llvm/test/MC/RISCV/invalid-attribute.s
+++ llvm/test/MC/RISCV/invalid-attribute.s
@@ -7,10 +7,10 @@
 # RUN: not llvm-mc %s -triple=riscv64 -filetype=asm 2>&1 | FileCheck %s
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: bad arch string foo
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,g}
 
 .attribute arch, "rv32i2p0_y2p0"
-# CHECK: [[@LINE-1]]:18: error: bad arch string y2p0
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p0_y2p0', invalid standard user-level extension 'y'
 
 .attribute stack_align, "16"
 # CHECK: [[@LINE-1]]:25: error: expected numeric constant
Index: llvm/test/MC/RISCV/attribute-with-insts.s
===
--- llvm/test/MC/RISCV/attribute-with-insts.s
+++ llvm/test/MC/RISCV/attribute-with-insts.s
@@ -10,7 +10,7 @@
 # RUN:   | llvm-objdump --triple=riscv64 -d -M no-aliases - \
 # RUN:   | FileCheck -check-prefix=CHECK-INST %s
 
-.attribute arch, "rv64i2p0_m2p0_a2p0_d2p0_c2p0"
+.attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 # CHECK-INST: lr.w t0, (t1)
 lr.w t0, (t1)
Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -9,9 +9,6 @@
 .attribute arch, "rv32i2"
 # CHECK: attribute  5, "rv32i2p0"
 
-.attribute arch, "rv32i2p"
-# CHECK: attribute  5, "rv32i2p0"
-
 .attribute arch, "rv32i2p0"
 # CHECK: attribute  5, "rv32i2p0"
 
@@ -33,14 +30,14 @@
 .attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
-.attribute arch, "rv32ima2p_fdc"
+.attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 .attribute arch, "rv32ib"
 # CHECK: attribute  5, "rv32i2p0_b0p93_zba0p93_zbb0p93_zbc0p93_zbe0p93_zbf0p93_zbm0p93_zbp0p93_zbr0p93_zbs0p93_zbt0p93"
 
 .attribute arch, "rv32iv"
-# CHECK: attribute  5, "rv32i2p0_v0p10"
+# CHECK: attribute  5, "rv32i2p0_v0p10_zvlsseg0p10"
 
 .attribute arch, "rv32izba"
 # CHECK: attribute  5, "rv32i2p0_zba0p93"
Index: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
===
--- llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
+++ llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
@@ -11,9 +11,11 @@
 //===--===//
 
 #include "RISCVTargetStreamer.h"
+#include "RISCVBaseInfo.h"
 #include "RISCVMCTargetDesc.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 
 using namespace llvm;
 
@@ -43,57 +45,14 @@
   else
 emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);
 
-  std::string Arch = "rv32";
-  if (STI.hasFeature(RISCV::Feature64Bit))
-Arch = "rv64";
-  if (STI.hasFeature(RISCV::FeatureRV32E))
-Arch += "e1p9";
-  else
-Arch += "i2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtM))
-Arch += "_m2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtA))
-Arch += "_a2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtF))
-Arch += "_f2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtD))
-Arch += "_d2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtC))
-Arch += "_c2p0";
-  if (STI.hasFeature(RISCV::FeatureStdExtB))
-Arch += "_b0p93";
-  if (STI.hasFeature(RISCV::FeatureStdExtV))
-Arch += "_v0p10";
-  if (STI.hasFeature(RISCV::FeatureExtZfh))
-Arch += "_zfh0p1";
-  if (STI.hasFeature(RISCV::FeatureExtZba))
-Arch += "_zba0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbb))
-Arch += "_zbb0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbc))
-Arch += "_zbc0p93";
-  if (STI.hasFeature(RISCV::FeatureExtZbe))
-Arch += "_zbe0p93";
-  if 

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

[Zakk Chen via Phabricator via cfe-commits]

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:   | FileCheck -check-prefix=CHECK-LP64D %s

Why do we need to modify here?


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

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

[Kito Cheng via Phabricator via cfe-commits]

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 good spot to sort it after all 
> extensions have been added. Are all the extensions added from the parse 
> functions?
All extension are added in parse function, but there might require a lookup and 
add if not found loop when implementing (more complete*) implied extension rule 
in future, so using ordered container would make this easier.

* We don't implement full implied extension rule, like f implied zicsr, d 
implied f...


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

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

[Kito Cheng via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D105168

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Driver/ToolChains/Arch/RISCV.cpp
  clang/test/Driver/riscv-abi.c
  clang/test/Driver/riscv-arch.c
  llvm/include/llvm/Support/RISCVISAInfo.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/invalid-attribute.s

Index: llvm/test/MC/RISCV/invalid-attribute.s
===
--- llvm/test/MC/RISCV/invalid-attribute.s
+++ llvm/test/MC/RISCV/invalid-attribute.s
@@ -7,10 +7,10 @@
 # RUN: not llvm-mc %s -triple=riscv64 -filetype=asm 2>&1 | FileCheck %s
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: bad arch string foo
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,g}
 
 .attribute arch, "rv32i2p0_y2p0"
-# CHECK: [[@LINE-1]]:18: error: bad arch string y2p0
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p0_y2p0', invalid standard user-level extension 'y'
 
 .attribute stack_align, "16"
 # CHECK: [[@LINE-1]]:25: error: expected numeric constant
Index: llvm/test/MC/RISCV/attribute-with-insts.s
===
--- llvm/test/MC/RISCV/attribute-with-insts.s
+++ llvm/test/MC/RISCV/attribute-with-insts.s
@@ -10,7 +10,7 @@
 # RUN:   | llvm-objdump --triple=riscv64 -d -M no-aliases - \
 # RUN:   | FileCheck -check-prefix=CHECK-INST %s
 
-.attribute arch, "rv64i2p0_m2p0_a2p0_d2p0_c2p0"
+.attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 # CHECK-INST: lr.w t0, (t1)
 lr.w t0, (t1)
Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -9,9 +9,6 @@
 .attribute arch, "rv32i2"
 # CHECK: attribute  5, "rv32i2p0"
 
-.attribute arch, "rv32i2p"
-# CHECK: attribute  5, "rv32i2p0"
-
 .attribute arch, "rv32i2p0"
 # CHECK: attribute  5, "rv32i2p0"
 
@@ -33,14 +30,14 @@
 .attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
-.attribute arch, "rv32ima2p_fdc"
+.attribute arch, "rv32ima2p0_fdc"
 # CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
 
 .attribute arch, "rv32ib"
 # CHECK: attribute  5, "rv32i2p0_b0p93_zba0p93_zbb0p93_zbc0p93_zbe0p93_zbf0p93_zbm0p93_zbp0p93_zbr0p93_zbs0p93_zbt0p93"
 
 .attribute arch, "rv32iv"
-# CHECK: attribute  5, "rv32i2p0_v0p10"
+# CHECK: attribute  5, "rv32i2p0_v0p10_zvlsseg0p10"
 
 .attribute arch, "rv32izba"
 # CHECK: attribute  5, "rv32i2p0_zba0p93"
Index: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
===
--- llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -35,6 +35,7 @@
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 #include "llvm/Support/TargetRegistry.h"
 
 #include 
@@ -50,6 +51,10 @@
 STATISTIC(RISCVNumInstrsCompressed,
   "Number of RISC-V Compressed instructions emitted");
 
+namespace llvm {
+extern const SubtargetFeatureKV RISCVFeatureKV[RISCV::NumSubtargetFeatures];
+}
+
 namespace {
 struct RISCVOperand;
 
@@ -2027,113 +2032,39 @@
 
   if (Tag == RISCVAttrs::ARCH) {
 StringRef Arch = StringValue;
-if (Arch.consume_front("rv32"))
+for (auto Feature : RISCVFeatureKV) {
+  if (llvm::RISCVISAInfo::isSupportedExtension(
+  Feature.Key, /* CheckExperimental */ true)) {
+clearFeatureBits(Feature.Value, Feature.Key);
+  }
+}
+
+llvm::RISCVISAInfo ISAInfo;
+if (auto E =
+ISAInfo.parse(StringValue, /* EnableExperimentalExtension */ true,
+  /* ExperimentalExtensionVersionCheck */ false)) {
+  std::string Buffer;
+  raw_string_ostream OutputErrMsg(Buffer);
+  handleAllErrors(std::move(E), [&](llvm::StringError ) {
+OutputErrMsg << "invalid arch name '" << Arch << "', "
+ << ErrMsg.getMessage();
+  });
+
+  return Error(ValueExprLoc, OutputErrMsg.str());
+}
+
+for (auto Feature : RISCVFeatureKV) {
+  if (ISAInfo.hasExtension(Feature.Key)) {
+setFeatureBits(Feature.Value, Feature.Key);
+  }
+}
+
+if (ISAInfo.getXLen() == 32)
   clearFeatureBits(RISCV::Feature64Bit, "64bit");
-else if 

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

[luxufan via Phabricator via cfe-commits]

StephenFan added inline comments.



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

This lambda function will keep alive out of current scope.
Maybe the `[&]` should change to `[ExtName]`




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

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

[Craig Topper via Phabricator via cfe-commits]

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 after all 
extensions have been added. Are all the extensions added from the parse 
functions?



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:279
+  Features.push_back("+experimental-zvamo");
+} else if (isExperimentalExtension(ExtName))
+  Features.push_back(Args.MakeArgString("+experimental-" + ExtName));

Consistently use curly braces on all of the blocks



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:303
+
+  if (MajorStr.size() && In.consume_front("p")) {
+MinorStr = std::string(In.take_while(isDigit));

MajorStr.size() -> !MajorStr.empty()



Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:55
+namespace llvm {
+extern const SubtargetFeatureKV RISCVFeatureKV[];
+}

Can this be `extern const SubtargetFeatureKV 
RISCVFeatureKV[RISCV::NumSubtargetFeatures];` then I think you can get rid of 
the ArrayRef? 


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

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

[Kito Cheng via Phabricator via cfe-commits]

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 wrote:
> I think maybe we could use the RISCVISAInfo to parse `March` here now?
Good catch, current checking mechanism will broken once we implement `zdinx`...


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

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

[Zakk Chen via Phabricator via cfe-commits]

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 maybe we could use the RISCVISAInfo to parse `March` here now?



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:312
+  "minor version number missing after 'p' for extension '%s'",
+  Ext.str().c_str());
+}

Would you consider to use Twine as argument here to make code more consistent?



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:400
+if (Add) {
+
+  for (auto const  :

redundant line


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

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

[Luís Marques via Phabricator via cfe-commits]

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: llvm/lib/Support/RISCVISAInfo.cpp:24-38
+// Represents the major and version number components of a RISC-V extension
+struct RISCVExtensionVersion {
+  unsigned Major;
+  unsigned Minor;
+};
+
+struct RISCVSupportedExtensionInfo {

Use `///` to add to the generated documentation?



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:112
+
+// Helper function for fiilter SupportedExtensionInfos by name.
+static auto filterSupportedExtensionInfosByName(StringRef ExtName) {

fiilter -> filtering



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:190-191
+  if (Pos == StringRef::npos)
+// If got an unknown extension letter, then give it an alphabetical
+// order, but after all known standard extension.
+Rank = AllStdExts.size() + (Ext - 'a');

got -> we got. extension -> extensions.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:235
+// Compare function for extension.
+// Only compare the extension name, ignore version comparesion.
+bool RISCVISAInfo::compareExtension(const std::string ,

comparesion -> comparison.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:260
+
+  // If the rank is same, it must be sorted by lexical order.
+  return LHS < RHS;

lexicographic order


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