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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
