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:
> 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.
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
