[PATCH] D63936: [ARM] Minor fixes in command line option parsing

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

I will let Oliver finish the review (I am off for a few days), just some drive-by comments.



================
Comment at: llvm/lib/Support/ARMTargetParser.cpp:412
 
-  if (Extensions & AEK_CRC)
-    Features.push_back("+crc");
-  else
-    Features.push_back("-crc");
-
-  if (Extensions & AEK_DSP)
-    Features.push_back("+dsp");
-  else
-    Features.push_back("-dsp");
-
-  if (Extensions & AEK_FP16FML)
-    Features.push_back("+fp16fml");
-  else
-    Features.push_back("-fp16fml");
-
-  if (Extensions & AEK_RAS)
-    Features.push_back("+ras");
-  else
-    Features.push_back("-ras");
-
-  if (Extensions & AEK_DOTPROD)
-    Features.push_back("+dotprod");
-  else
-    Features.push_back("-dotprod");
+  for (const auto AE : ARCHExtNames) {
+    if ((Extensions & AE.ID) == AE.ID && AE.Feature)
----------------
This could be a little local helper function, share the code, as exactly the same is done in `ARM::appendArchExtFeatures`


================
Comment at: llvm/unittests/Support/TargetParserTest.cpp:574
 
-  Extensions[ARM::AEK_CRC]        = { "+crc",       "-crc" };
-  Extensions[ARM::AEK_DSP]        = { "+dsp",       "-dsp" };
+  for (auto &Ext : ARM::ARCHExtNames) {
+    if (Ext.Feature && Ext.NegFeature)
----------------
I like this  approach.


================
Comment at: llvm/unittests/Support/TargetParserTest.cpp:580
+
   Extensions[ARM::AEK_HWDIVARM]   = { "+hwdiv-arm", "-hwdiv-arm" };
   Extensions[ARM::AEK_HWDIVTHUMB] = { "+hwdiv",     "-hwdiv" };
----------------
but the fact that we have these still here, I guess that means they are not present in the table. Can we add them too? I guess that's why you've added `fp.dp`.


================
Comment at: llvm/unittests/Support/TargetParserTest.cpp:585
 
-  EXPECT_FALSE(AArch64::getExtensionFeatures(ARM::AEK_INVALID, Features));
+  EXPECT_FALSE(ARM::getExtensionFeatures(ARM::AEK_INVALID, Features));
 
----------------
Oops, thanks for fixing! :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63936