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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

In https://reviews.llvm.org/D31418#722861, @ab wrote:

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


Yes, but only because of the test cases. I can remove the dependency fairly easily.



================
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.
----------------
ab wrote:
> Can we emit this in the .inc instead?
I expect so. I'll look into it


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:485
+    const MachineFunction &MF) {
+  ForCodeSize = MF.getFunction()->optForSize();
+  AvailableFeatures = computeAvailableFeatures(&MF, &STI);
----------------
ab wrote:
> 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.
My first attempt tried to deal with optsize/optnone inside <Target>TargetMachine::getSubtargetImpl() so that we didn't need this hook but I had problems with getting the wrong subtarget during the instruction selector. Unfortunately, I didn't manage to get to the bottom of that and I needed to move on to some dependent work so I had to fall back on this approach.


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:486
+  ForCodeSize = MF.getFunction()->optForSize();
+  AvailableFeatures = computeAvailableFeatures(&MF, &STI);
+}
----------------
ab wrote:
> 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.
> At least for features, this should be identical across functions that we select using the same subtarget 

Agreed. There's a subtarget instance for each unique feature string.

> (and nothing uses the MF anyway)

X86 needs it for NotWin64WithoutFP

> 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.

I originally wanted it in the ctor but had to move it out to support the predicates involving function attributes (e.g. optsize/optnone) and for X86's NotWin64WithoutFP.


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:79
+  PredicateBitset AvailableFeatures;
+  PredicateBitset getAvailableFeatures() const { return AvailableFeatures; }
+  PredicateBitset
----------------
ab wrote:
> If we're going to access things like OptForSize directly, I'm not sure a getter for the features is valuable.
We don't normally access OptForSize directly in this implementation. It's used by ComputeAvailableFeatures() to initialize a feature bit but after that call it's unused. The only reason we store it in the class is so that the C++ code from the Predicate<> subclasses can be dropped in to ComputeAvailableFeatures() for that initialization.


================
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;
+
----------------
ab wrote:
> 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?
I agree but I think it will need to be done by moving users of the other one to this version. X86 defines 104 predicates of which 85 end up in tablegen-erated at the moment. The remainder are on rules that we don't import yet. The other implementation is limited to 64 features so we can't use that.


https://reviews.llvm.org/D31418