[PATCH] D59002: Use bitset for assembler predicates

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:10484
       // Don't report the same set of features twice.
-      if (FeatureMissesSeen.count(MissingFeatures))
+      if (FeatureMissesSeen.count(MissingFeatures.to_ullong()))
         break;
----------------
craig.topper wrote:
> This seems to be making the assumption that ARM feature bits fit in a unisgned long long?
Yes, if exceptions enabled it would raise an exception. The set cannot be constructed from a FeatureBitset since it does not have comparison operators.
I would add an assert, but unfortunately cannot enumerate all features. The solution can be to use a SmallVector, but as long as it fits 64 bit this seems to be an overkill.


================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:2834
   OS << "  struct OperandMatchEntry {\n";
-  OS << "    " << getMinimalTypeForEnumBitfield(Info.SubtargetFeatures.size())
-               << " RequiredFeatures;\n";
+  OS << "    " << "unsigned RequiredFeaturesIdx;\n";
   OS << "    " << getMinimalTypeForRange(MaxMnemonicIndex)
----------------
craig.topper wrote:
> Can we bound this to less than a full unsigned?
Yes, 8 bit is enough.


================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:3366
+    OS << "  {";
+    for (const auto &Feature : FeatureBitset) {
+      const auto &I = Info.SubtargetFeatures.find(Feature);
----------------
craig.topper wrote:
> How many entries end up in this table on different targets? Since we're tablegening this, can we use hex contants and use FeatureBitArray instead of invoking the std::initializer_list constructor? That probably depends on how many entries end up in the table.
Asm matcher:

MSP430:0
AArch64:32
ARM:88
WebAssembly:7
Hexagon:11
BPF:0
Sparc:5
AMDGPU:55
Mips:137
PowerPC:10
SystemZ:28
Lanai:0
X86:5

MCCodeEmitter:

MSP430:0
AArch64:28
ARM:81
WebAssembly:8
Hexagon:11
BPF:0
Sparc:5
AMDGPU:68
AMDGPU:0
Mips:149
PowerPC:0
SystemZ:29
Lanai:0

Do you think we need FeatureBitArray here? It has a cost as well, every time one calls getAsBitset().


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59002/new/

https://reviews.llvm.org/D59002