labrinea added a comment. In D63936#1563872 <https://reviews.llvm.org/D63936#1563872>, @ostannard wrote:
> > The second change this patch makes > > Could this be spilt into two patches? Looking at D62998 <https://reviews.llvm.org/D62998> more carefully I realized that we deliberately favor cpu extensions over `-mfpu`: > That in turn caused an ordering problem when handling -mcpu=foo+bar > together with -mfpu=something_that_turns_off_bar. To fix that, I've > arranged that the +bar suffixes on the end of -mcpu and -march > cause feature names to be put into a separate vector which is > concatenated after the output of getFPUFeatures. I am now in doubt about my changes in `clang/lib/Driver/ToolChains/Arch/ARM.cpp`. Imagine this case: `-mcpu=cortex-a73 -mfpu=crypto-neon-fp-armv8` According to the table in ARMTargetParser, cortex-a73 doesn't have crypto, therefore the `-crypto` feature gets in the vector, but then we explicitly ask for it through the mfpu option. What is supposed to win here? FYI this a test case from `clang/test/Driver/arm-cortex-cpus.c`. An obvious workaround is to add the crypto extension for cortex-a73 (and any other entry which is missing it) in the table. Maybe @simon_tatham could shed some light here? ================ 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) ---------------- 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. ================ Comment at: llvm/unittests/Support/TargetParserTest.cpp:580 + Extensions[ARM::AEK_HWDIVARM] = { "+hwdiv-arm", "-hwdiv-arm" }; Extensions[ARM::AEK_HWDIVTHUMB] = { "+hwdiv", "-hwdiv" }; ---------------- SjoerdMeijer wrote: > but the fact that we have these still here, I guess that means they are not > present in the table. Can we add them too? I guess that's why you've added > `fp.dp`. Unfortunately we can't, meaning that the table is supposed to contain feature names that are valid command line options for `mcpu`, `march` and those are clearly not. Or at least, that's my understanding of it. 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
