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

[Alexandros Lamprineas via Phabricator via cfe-commits]

labrinea 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)
----------------
ostannard wrote:
> labrinea wrote:
> > ostannard wrote:
> > > 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.
> > The target features explicitly specified on the command line are handled by `ARM::appendArchExtFeatures` (see [[ https://reviews.llvm.org/D64048 | D64048 ]]). That said, yes, -march=...+mve+fp does enable scalar floating point and integer-only mve, but doesn't enable mve with floating point. I could possibly add such a test on that revision.
> > 
> > On the other hand, `ARM::getExtensionFeatures` cannot tell the difference between the two configurations you describe, and it's not possible to do so, because they are represented on a bitmap returned from `ARM::getDefaultExtensions`, which reads the table. That said, if there was an entry in the table containing `AEK_FP | AEK_SIMD` that would enable mve.fp. However, I don't think this is a problem in practice. My understanding is that the table indicates FPU support with `FK_*`, and Extension support with `AEK_*`.  That said, I believe AEK_FP is only used for command line option handling.
> > 
> > Maybe a fix for this problem would be to replace `AEK_FP | AEK_SIMD` with something like `AEK_MVE_FP` but then we wouldn't be able to do what is proposed in [[ https://reviews.llvm.org/D64048 | D64048 ]].
> Is this system (in particular the behaviour added in D64048) going to be able to handle all of the other dependencies between architectural features? For example, MVE also depends on the DSP extension, but `-march=armv8.1-m.main+mve+nodsp` currently defines both __ARM_FEATURE_MVE and __ARM_FEATURE_DSP.
No, `-march=armv8.1-m.main+mve+nodsp` doesn't turn off neither mve nor dsp and it looks like a bug if they depend on each other. It seems you are right, the code in `ARMTargetInfo::handleTargetFeatures` enables both when mve is set:

```
} else if (Feature == "+mve") {
  DSP = 1;
  MVE |= MVE_INT;
} else if (Feature == "+mve.fp") {
  DSP = 1;
  HasLegalHalfType = true;
  FPU |= FPARMV8;
  MVE |= MVE_INT | MVE_FP;
  HW_FP |= HW_FP_SP | HW_FP_HP;
}
```
If there's a dependency then it should be present in the table of target parser. Then the above command would turn both off. I'll update the table and add some tests in [[ https://reviews.llvm.org/D64048 | D64048 ]].


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63936