[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:
> > 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.
Yes.

I just don't see why it should take a set - I think that's confusing, since the pre-condition would have to be that this set has only one bit on.
But if you have a good reason for it to take a set, I'm ok with that too.

http://reviews.llvm.org/D10542

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