[PATCH] Replace uint64_t representation of Features with FeatureBitset (std::bitset) in a few more places

Ranjeet Singh ranjeet.singh at arm.com
Mon Jun 22 03:46:42 PDT 2015

Thanks for the review Michael.

Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:2628
@@ -2621,3 +2627,3 @@
   uint64_t ErrorInfoIgnore;
-  uint64_t ErrorInfoMissingFeature = 0; // Init suppresses compiler warnings.
+  FeatureBitset ErrorInfoMissingFeature; // Init suppresses compiler warnings.
   unsigned Match[4];
mkuper wrote:
> Can probably remove the comment. :-)
Will do :)

Comment at: utils/TableGen/AsmMatcherEmitter.cpp:2257
@@ -2256,3 +2256,3 @@
      << "// instruction matching.\n"
-     << "static const char *getSubtargetFeatureName(uint64_t Val) {\n";
+     << "static const char *getSubtargetFeatureName(FeatureBitset Val) {\n";
   if (!Info.SubtargetFeatures.empty()) {
mkuper wrote:
> I'm not sure we need to make this take a bitset. 
> It's supposed to take a single feature value, right?
> With the previous interface, a uint64_t stood for both a single value and a set, but now the two are different. We can still use a uint64_t for a single value, however, it's just that it's now an ordinal number instead of a bit.
The call sites of this function have always passed a bitset, here is a snippet of the previous code that was calling this function:

    uint64_t Mask = 1;
    for (unsigned i = 0; i < (sizeof(ErrorInfo)*8-1); ++i) {
      if (ErrorInfo & Mask) {
        Msg += " ";
        Msg += getSubtargetFeatureName(ErrorInfo & Mask);
      Mask <<= 1;
If it's supposed to take the bit position then shouldn't there be another variable to keep count on which bit they're currently on in the loop (e.g. 'int BitPos = 0') and pass that variable to 'getSubtargetFeatureName' ? 

Maybe it would be good to change the name of the argument to 'Feature' instead of having it as 'Val' to make it more clear.



More information about the llvm-commits mailing list