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

[Michael Kuperstein]

================
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()) {
----------------
rs wrote:
> 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.
Well, it's not really passing a bitset. 
I mean, it is passing a bitset, but it's passing a bitset with a single bit set - since that's what Mask is. 
In any case, the function definitely doesn't expect anything else. E.g. this is what we have for x86:


```
static const char *getSubtargetFeatureName(uint64_t Val) {
  switch(Val) {
  case Feature_HasAVX512: return "AVX-512 ISA";
  case Feature_HasCDI: return "AVX-512 CD ISA";
  case Feature_HasPFI: return "AVX-512 PF ISA";
  case Feature_HasERI: return "AVX-512 ER ISA";
  case Feature_HasDQI: return "AVX-512 DQ ISA";
  case Feature_HasBWI: return "AVX-512 BW ISA";
  case Feature_HasVLX: return "AVX-512 VL ISA";
  case Feature_Not64BitMode: return "Not 64-bit mode";
  case Feature_In64BitMode: return "64-bit mode";
  case Feature_In16BitMode: return "16-bit mode";
  case Feature_Not16BitMode: return "Not 16-bit mode";
  case Feature_In32BitMode: return "32-bit mode";
  default: return "(unknown)";
  }
}
```

I think you can just rewrite the caller loop as something like (I'm not sure I don't have any off-by-ones here):


```
for (unsigned i = 0; i < ErrorInfo.size(); ++i) {
  if (ErrorInfo[i]) {
    Msg += " ";
    Msg += getSubtargetFeatureName(Mask);
  }
}
```

And, yes, changing the name from Val to Feature is probably a good idea.

http://reviews.llvm.org/D10542

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/