[PATCH] D119917: [Support] Add CSKY target parser and attributes parser

[David Spickett via Phabricator via llvm-commits]

DavidSpickett added inline comments.


================
Comment at: llvm/unittests/Support/CSKYAttributeParserTest.cpp:110-111
+  EXPECT_TRUE(testTagString(4, "Tag_CSKY_ARCH_NAME"));
+  EXPECT_TRUE(
+      testAttributeString(4, "ck860", CSKYAttrs::CSKY_ARCH_NAME, "ck860"));
+}
----------------
As this attribute is a string, so it doesn't make sense to exhaustively check every arch name here since all this is doing is pulling a string not checking what it's value is.

Correct? (same for cpu test below)


================
Comment at: llvm/unittests/Support/CSKYAttributeParserTest.cpp:124
+                               CSKYAttrs::DSP_VERSION_EXTENSION));
+  EXPECT_TRUE(testAttributeInt(8, 2, CSKYAttrs::CSKY_DSP_VERSION,
+                               CSKYAttrs::DSP_VERSION_2));
----------------
Does this need to check the "Error" case?

Same for the others that have use an "Error" string.


================
Comment at: llvm/unittests/Support/CSKYAttributeParserTest.cpp:194
+                               CSKYAttrs::FPU_HARDFP_DOUBLE));
+}
----------------
Add a test for the hardFP unknown error.


================
Comment at: llvm/unittests/Support/CSKYTargetParserTest.cpp:89
+INSTANTIATE_TEST_SUITE_P(
+    CSKYCPUTestsPart1, CSKYCPUTestFixture,
+    ::testing::Values(
----------------
Seems like llvm updated the copy of gtest since I refactored the Arm tests. It's letting you have 154 tests in one block, used to max out at 50.

In which case this can just be `CSKYCPUTests`.


================
Comment at: llvm/unittests/Support/CSKYTargetParserTest.cpp:1045
+
+TEST(TargetParserTest, testCSKYExtension) {
+  EXPECT_TRUE(testCSKYExtension("ck801", "trust"));
----------------
What is the purpose of this? Seems like it would be covered by `CSKYCPUTestsPart1`.


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

https://reviews.llvm.org/D119917