[PATCH] D26343: [RFC] Refactor TargetParserTests

[jojo.ma via Phabricator via llvm-commits]

jojo added inline comments.


================
Comment at: unittests/Support/TargetParserTest.cpp:45
+  else {
+    unsigned BaseExt = ARM::AEK_CRC | ARM::AEK_CRYPTO | ARM::AEK_DSP |
+                       ARM::AEK_HWDIVARM | ARM::AEK_HWDIV | ARM::AEK_MP |
----------------
rengolin wrote:
> I have the impression that the string representation is redundant.
> 
> Instead of generating the string representation of the default extensions, then comparing to the given string, I think you should just pass the values as unsigned ex. (CRC | DSP), then compare with the result from `getDefaultExtensions`.
Good idea! This make the code more simple.Thanks.


================
Comment at: unittests/Support/TargetParserTest.cpp:253
+
+TEST(TargetParserTest, testARMExtension) {
+  EXPECT_FALSE(testARMExtension("arm2", 0, "crc"));
----------------
rengolin wrote:
> I like these tests, but would be good to make them check for a feature that is close enough, but not implemented.
> 
> For example, vfp on pre-v5, vfpv3 on pre-v7, vfpv4 on cortex-a8, etc.
> 
> Also, the v7M cores are redundant, you only need to test one v6M (M0) and one v7M (M3).
Indeed. Make sense.
I don't know enough on ARM architecture. So I have to make them as close as possible according to information collected from the net.
There may still be a lot of inappropriate,please let me know. 


================
Comment at: unittests/Support/TargetParserTest.cpp:541
+                              ARMBuildAttrs::CPUArch::v8_A));
+  EXPECT_TRUE(testAArch64Arch("armv8.2-a", "generic", "v8.2a",
+                              ARMBuildAttrs::CPUArch::v8_A));
----------------
rengolin wrote:
> We now have v8M and v8R.
AFAIK, both of them are 32bit. 
And v8R has been supported in ARMTargetParser. 
There is still no support in ARM backend for v8M, 
Should  we add it to targetparser after it is actually
supported in the backend?


================
Comment at: unittests/Support/TargetParserTest.cpp:563
+  EXPECT_FALSE(testAArch64Extension("generic", ARM::AK_ARMV8_1A, "ras"));
+  EXPECT_FALSE(testAArch64Extension("generic", ARM::AK_ARMV8_2A, "fp"));
 }
----------------
rengolin wrote:
> I think v8.2A has FP by default. If this test returns FALSE, then there's something wrong with the code. :)
Yes,indeed. There is a incorrect index problem in this test.
No errors printed make me missed the wrong flags here.


https://reviews.llvm.org/D26343