This patch replaces uses of uint64_t to represent target features with FeatureBitset in a few places where it hasn't been replaced.
Details
Diff Detail
Event Timeline
Thanks!
I originally only converted the subtarget features, but not the available features, since, at least on x86, only a small subset of subtarget features actually participate in that, so it didn't look like it was running out. There's was no good reason for that, though.
Anyway, lgtm with a couple of nits.
lib/Target/X86/AsmParser/X86AsmParser.cpp | ||
---|---|---|
2628 | Can probably remove the comment. :-) | |
utils/TableGen/AsmMatcherEmitter.cpp | ||
2257 | I'm not sure we need to make this take a bitset. 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. |
Thanks for the review Michael.
lib/Target/X86/AsmParser/X86AsmParser.cpp | ||
---|---|---|
2628 | Will do :) | |
utils/TableGen/AsmMatcherEmitter.cpp | ||
2257 | 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. |
utils/TableGen/AsmMatcherEmitter.cpp | ||
---|---|---|
2257 | Well, it's not really passing a bitset. 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. |
utils/TableGen/AsmMatcherEmitter.cpp | ||
---|---|---|
2257 | Just to confirm you want the function to be changed back to how it was originally to accept a uint64_t type?
OK. |
utils/TableGen/AsmMatcherEmitter.cpp | ||
---|---|---|
2257 | 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. |
Hi Michael,
I've made the changes, please could you have another look?
Thanks,
Ranjeet
Hm. I've read the patch more carefully, and I'm actually somewhat confused now.
Each target has two enumerations:
- SubtargetFeatureFlag - this is used, I think, only by the AsmParser, and has elements that look like Feature_HasAVX512.
This one still lives in a regular bitfield, not a bitset, so the elements of the enum are of them form "1 << C".
If I'm not mistaken, ComputeAvailableFeatures() returns a set of these values.
- A nameless enum used everywhere else, which has elements that look like X86::FeatureAVX512.
I've changed this enum in my original patch, and now these are ordinal values (1, 2, 3... etc.), instead of powers of 2.
Now, it looks like what you did was to change everything that used bitfields containing SubtargetFeatureFlag to use a FeatureBitset instead. But the values themselves are still powers of 2, so you are still bound by the limits of the underlying type.
Is that right? If it is, then what is the goal of this patch?
If not, then I misunderstood the patch. Could you explain again what it's trying to achieve?
I missed the fact that the SubtargetFeatureFlags enums are still powers of 2, I should have changed it so that they are ordinal numbers the same way you did it in your original patch because it's possible that the enum list could grow to >64 entries.
Thanks,
Ranjeet
Hi Michael,
I've update the patch so that SubtargetFeatureFlags enums are ordinal numbers rather than powers of 2. Please could you review the patch?
Thanks,
Ranjeet
Ok, this looks right - with a couple of new nits. :-)
lib/Target/ARM/AsmParser/ARMAsmParser.cpp | ||
---|---|---|
9929 | Extra space here? | |
lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp | ||
804–806 | Are you sure about the "-1"? | |
lib/Target/X86/AsmParser/X86AsmParser.cpp | ||
2553 | Same as above re -1. |
Hi Michael,
Extra space here?
Nice spot, I've removed it.
Are you sure about the "-1"?
(Then again, I don't understand why the original -1 was there either...)
No, I'm not sure about it, it was inherited from the previous code. I can't find a reason as to why it was there so I've removed it.
Thanks,
Ranjeet
Extra space here?