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