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

[Ranjeet Singh]

================
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:
> 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.
Just to confirm you want the function to be changed back to how it was originally to accept a uint64_t type?

>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):

OK.

http://reviews.llvm.org/D10542

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