[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.
http://reviews.llvm.org/D10542
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list