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

[Alexandros Lamprineas via Phabricator via cfe-commits]

labrinea added a comment.

In D63936#1563872 <https://reviews.llvm.org/D63936#1563872>, @ostannard wrote:

> > The second change this patch makes
>
> Could this be spilt into two patches?


Looking at D62998 <https://reviews.llvm.org/D62998> more carefully I realized that we deliberately favor cpu extensions over `-mfpu`:

> That in turn caused an ordering problem when handling -mcpu=foo+bar
>  together with -mfpu=something_that_turns_off_bar. To fix that, I've
>  arranged that the +bar suffixes on the end of -mcpu and -march
>  cause feature names to be put into a separate vector which is
>  concatenated after the output of getFPUFeatures.

I am now in doubt about my changes in `clang/lib/Driver/ToolChains/Arch/ARM.cpp`. Imagine this case:
`-mcpu=cortex-a73 -mfpu=crypto-neon-fp-armv8`
According to the table in ARMTargetParser, cortex-a73 doesn't have crypto, therefore the `-crypto` feature gets in the vector, but then we explicitly ask for it through the mfpu option. What is supposed to win here? FYI this a test case from `clang/test/Driver/arm-cortex-cpus.c`. An obvious workaround is to add the crypto extension for cortex-a73 (and any other entry which is missing it) in the table.

Maybe @simon_tatham could shed some light here?



================
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)
----------------
SjoerdMeijer wrote:
> This could be a little local helper function, share the code, as exactly the same is done in `ARM::appendArchExtFeatures`
We are not doing exactly the same thing in these functions. Here we extract features out of a bitmap, which is a map containing a bitwise OR of separate feature bitmasks. If a bitmask that corresponds to a known feature is present - and here I mean all the bits of that mask are present - then we push the feature, otherwise not. 

In `ARM::appendArchExtFeatures` we compare a given bitmask, which corresponds to a specific feature, against all the known bitmasks (individual features) one by one. In contrast to `ARM::getExtensionFeatures` here we also handle negative features, and that means the prohibition of a feature can disable other features that depend on it as well.


================
Comment at: llvm/unittests/Support/TargetParserTest.cpp:580
+
   Extensions[ARM::AEK_HWDIVARM]   = { "+hwdiv-arm", "-hwdiv-arm" };
   Extensions[ARM::AEK_HWDIVTHUMB] = { "+hwdiv",     "-hwdiv" };
----------------
SjoerdMeijer wrote:
> 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`.
Unfortunately we can't, meaning that the table is supposed to contain feature names that are valid command line options for `mcpu`, `march` and those are clearly not. Or at least, that's my understanding of it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63936