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:
> > 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 ]].
================
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)
----------------
ostannard wrote:
> 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.
Surely, but is the purpose of this test to check that the table is correct, or
that `ARM::getExtensionFeatures` reads the table correctly? I'd say the latter.
Also, with this change we won't need to update the test every time there's a
new entry in the table.
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
