[PATCH] D144353: [lld][RISCV] Avoid error when encountering unrecognised ISA extensions/versions in RISC-V attributes

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:545
+                               "extension lacks version in expected format");
+    unsigned long long MajorVersion, MinorVersion;
+    if (getAsUnsignedInteger(MinorVersionStr, 10, MinorVersion))
----------------
asb wrote:
> MaskRay wrote:
> > `getExtensionVersion` uses `unsigned` instead of `unsigned long long`.
> `unsigned long long` needs to be used for `StringRef::getAsUnsignedInteger`. Changing to `unsigned` gives:
> 
> ```
> /home/asb/llvm-project/llvm/lib/Support/RISCVISAInfo.cpp:552:9: error: no matching function for call to 'getAsUnsignedInteger'
>     if (getAsUnsignedInteger(MinorVersionStr, 10, MinorVersion))
>         ^~~~~~~~~~~~~~~~~~~~
> /home/asb/llvm-project/llvm/include/llvm/ADT/StringRef.h:34:8: note: candidate function not viable: no known conversion from 'unsigned int' to 'unsigned long long &' for 3rd argument
>   bool getAsUnsignedInteger(StringRef Str, unsigned Radix,
>        ^
> ```
OK. We can use the template member function `StringRef::getAsInteger` to avoid the implicit `unsigned long long` to `unsigned` cast below.


================
Comment at: llvm/unittests/Support/RISCVISAInfoTest.cpp:1
+//===----- unittests/RISCVISAInfoTest.cpp ---------------------------------===//
+//
----------------
`//===-- ` or 3 dashes is more common


================
Comment at: llvm/unittests/Support/RISCVISAInfoTest.cpp:22
+TEST(ParseNormalizedArchString, RejectsUpperCase) {
+  for (auto Input : {"RV32", "rV64", "rv32i2P0", "rv64i2p0_A2p0"}) {
+    EXPECT_EQ(
----------------
s/auto/StringRef/

https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable


================
Comment at: llvm/unittests/Support/RISCVISAInfoTest.cpp:30
+TEST(ParseNormalizedArchString, RejectsInvalidBaseISA) {
+  for (auto Input : {"rv32", "rv64", "rv32j", "rv65i"}) {
+    EXPECT_EQ(
----------------
s/auto/StringRef/

https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable


================
Comment at: llvm/unittests/Support/RISCVISAInfoTest.cpp:38
+TEST(ParseNormalizedArchString, RejectsMalformedInputs) {
+  for (auto Input : {"rv64i2p0_", "rv32i2p0__a2p0", "rv32e2.0", "rv64e2p",
+                     "rv32i", "rv64ip1"}) {
----------------
s/auto/StringRef/

https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable


================
Comment at: llvm/unittests/Support/RISCVISAInfoTest.cpp:51
+  ASSERT_EQ(InfoRV32I.getExtensions().size(), 1UL);
+  ASSERT_TRUE(InfoRV32I.getExtensions().at("i") ==
+              (RISCVExtensionInfo{"i", 2, 0}));
----------------
Use `EXPECT_` for non-fatal checks.


================
Comment at: llvm/unittests/Support/RISCVISAInfoTest.cpp:67
+  ASSERT_EQ(InfoRV64I.getExtensions().size(), 1UL);
+  ASSERT_TRUE(InfoRV64I.getExtensions().at("i") ==
+              (RISCVExtensionInfo{"i", 2, 0}));
----------------
Use `EXPECT_` for non-fatal checks.


================
Comment at: llvm/unittests/Support/RISCVISAInfoTest.cpp:75
+  ASSERT_EQ(InfoRV64E.getExtensions().size(), 1UL);
+  ASSERT_TRUE(InfoRV64E.getExtensions().at("e") ==
+              (RISCVExtensionInfo{"e", 2, 0}));
----------------
Use `EXPECT_` for non-fatal checks.


================
Comment at: llvm/unittests/Support/RISCVISAInfoTest.cpp:86
+  ASSERT_EQ(Info.getExtensions().size(), 5UL);
+  ASSERT_TRUE(Info.getExtensions().at("i") == (RISCVExtensionInfo{"i", 5, 1}));
+  ASSERT_TRUE(Info.getExtensions().at("m") == (RISCVExtensionInfo{"m", 3, 2}));
----------------
Use `EXPECT_` for non-fatal checks.


================
Comment at: llvm/unittests/Support/RISCVISAInfoTest.cpp:101
+  RISCVISAInfo &Info = **MaybeISAInfo;
+  ASSERT_EQ(Info.getXLen(), 64U);
+  ASSERT_EQ(Info.getFLen(), 64U);
----------------
Use `EXPECT_` for non-fatal checks.


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

https://reviews.llvm.org/D144353