[PATCH] D63936: [clang][Driver][ARM] Favor -mfpu over default CPU features

[Oliver Stannard (Linaro) via Phabricator via llvm-commits]

ostannard added inline 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)
----------------
labrinea wrote:
> 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.
Does this correctly handle the combination of integer-only MVE with a scalar FPU? I think -march=...+mve+fp should enable AEK_FP and AEK_SIMD (+mve), but shouldn't enable +mve.fp.


================
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)
----------------
SjoerdMeijer wrote:
> I like this  approach.
I'm not sure this is a good idea - we are now testing the implementation by checking that it matches the table used by the implementation, so if there's a bug in the table this will still pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63936