[PATCH] D31418: [globalisel][tablegen] Import SelectionDAG's rule predicates and support the equivalent in GIRule.

[Ahmed Bougacha via Phabricator via llvm-commits]

ab added a comment.

Does this really depend on https://reviews.llvm.org/D31329?  Can you invert the dependency and put the tblgen emission bits in that patch?



================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelector.h:35-38
+/// Each InstructionSelector subclass should define a PredicateBitset class with:
+///   const unsigned MAX_SUBTARGET_PREDICATES = 192;
+///   using PredicateBitset = PredicateBitsetImpl<MAX_SUBTARGET_PREDICATES>;
+/// and updating the constant to suit the target.
----------------
Can we emit this in the .inc instead?


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:485
+    const MachineFunction &MF) {
+  ForCodeSize = MF.getFunction()->optForSize();
+  AvailableFeatures = computeAvailableFeatures(&MF, &STI);
----------------
We already specialize the selector (and everything else) per-subtarget.  Should we also treat 'optsize' and 'optnone' the same way?  We wouldn't need to do anything per-function then.

That might not be a good idea, but some of our features are already not hardware-related, and seem closer to 'optsize' than to other hardware features.


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:486
+  ForCodeSize = MF.getFunction()->optForSize();
+  AvailableFeatures = computeAvailableFeatures(&MF, &STI);
+}
----------------
At least for features, this should be identical across functions that we select using the same subtarget (and nothing uses the MF anyway).  Should this be in the ctor?  It's unfortunate they would be separate from the other predicates, but Subtarget features are the essence of the subtarget, it seems reasonable to associate them with the subtarget-specific selector.


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:79
+  PredicateBitset AvailableFeatures;
+  PredicateBitset getAvailableFeatures() const { return AvailableFeatures; }
+  PredicateBitset
----------------
If we're going to access things like OptForSize directly, I'm not sure a getter for the features is valuable.


================
Comment at: utils/TableGen/SubtargetFeatureInfo.cpp:107-119
+  OS << "PredicateBitset " << TargetName << ClassName << "::\n"
+     << FuncName << "(const MachineFunction *MF, const " << TargetName
+     << "Subtarget *Subtarget) const {\n";
+  OS << "  PredicateBitset Features;\n";
+  for (const auto &SF : SubtargetFeatures) {
+    const SubtargetFeatureInfo &SFI = SF.second;
+
----------------
It would be nice to use the same ComputeAvailableFeatures.  A std::bitset does let us use more than 64 features, but is any target anywhere near that?


https://reviews.llvm.org/D31418