[PATCH] D39034: [WIP][GlobalISel][TableGen] Optimize MatchTable for faster instruction selection

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:594-597
+  void clearImplicitMap() {
+    NextInsnVarID = 0;
+    InsnVariableIDs.clear();
+  };
----------------
qcolombet wrote:
> dsanders wrote:
> > qcolombet wrote:
> > > dsanders wrote:
> > > > I don't understand why this is needed.
> > > Admittedly, this is a hack around the fact that capturing a variable is not an action, but instead something that happen somewhat magically when we need to access something (defineInsnVar).
> > > 
> > > Now, how does this work:
> > > 1. The "predicates" are self contained now, i.e., they reference all the InsnID and OpIdx they are going to access. In particular, they don't rely on a Rule at emission time to get this information (though the information needs to be in sync for this to work, thus the bunch of asserts added) Thanks to that, we can check that two predicates are really identical (as opposed to checking the same thing, but on different variables).
> > > 2. When building the predicates we implicitly defines all the variables that are going to be accessed in Rule.
> > > 3. We clean the implicit def in Rule (this method)
> > > 4. When we emit Rule we check that the defined variables are equal to what we chose at build time for the predicate
> > > 
> > > If we skip #3, Rule is going to keep increasing the InsnVarID and the numbers we came up at build time won't reference the right thing.
> > > 
> > > Does it make sense? (look line 2842 for the call to that method)
> > If we're going to put InsnID/OpIdx in the matchers, why not use those and skip step 3 and 4?
> That's the long term plan, but it involves a bigger refactoring and I was not willing to pull that into this patch.
> Is that fine with you?
> 
> (Admittedly, I was not planning to do that refactoring any time soon!)
That makes sense to me but we should follow up with the refactor asap. In the mean-time, we should comment on the requirement that the numbering comes out the same and explain the plan to fix it.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:533
+  }
+  bool conditionMatches(const PredicateMatcher &Predicate) const;
+  bool conditions_empty() const { return Conditions.empty(); }
----------------
qcolombet wrote:
> dsanders wrote:
> > This function is a bit confusing. Going by it's name, I think it's trying to detect whether the Predicate is already in the Conditions array but the code only checks the last condition (which is enough given how we generate the table)
> Yeah, the name is not great (suggestion welcome :)), but the code does exactly what I wanted to. The incentive was to keep the building of groups cheap and the idea was if two rules don't have the same order in their predicates, they are not going to be grouped together. Although this is a weakness of the current grouping (in other words, it won't group thing that may have been commuted between two rules), the assumption is that the predicates are sorted in the same relative order and thus it should be that relevant.
> 
> => that's intentional :)
lastConditionMatches()? Alternatively, the caller could use `Predicate.isIdenticalTo(group.conditions_back())`


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:719-731
+    IPM_Opcode,
+    IPM_ImmPredicate,
+    IPM_NonAtomicMMO,
     OPM_Tie,
     OPM_ComplexPattern,
     OPM_IntrinsicID,
     OPM_Instruction,
----------------
qcolombet wrote:
> dsanders wrote:
> > What should be the relative priority of the IPM_* and OPM_* predicates. They act on different things so I don't think there's a meaningful one but I also don't think we should have an artificial one.
> I mainly followed what I believe was the layout of the table: Instruction then Operand. (General to Special.)
I think we'll get better tables if opcodes are allowed to have low priority. For example, binary operations can check for `reg <op> reg => reg`, and `reg <op> imm => reg` before testing the opcode.

That said, looking at it again it's not a problem since PredicateMatcher doesn't define a isHigherPriorityThan(). They're in OperandPredicateMatcher and InstructionPredicateMatcher so the two families are never compared.

Could you add to the comment saying the relative priority of IPM_* and OPM_* doesn't matter?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:3052
 
+std::vector<Matcher *> GlobalISelEmitter::OptimizeRules(
+    std::vector<RuleMatcher> &Rules,
----------------
qcolombet wrote:
> qcolombet wrote:
> > dsanders wrote:
> > > OptimizeRules -> optimizeRules
> > > 
> > > Also, I think this code will break if it ever manages to hoist a predicate that uses InsnID>0 or any OpIdx. There's currently no code to ensure an appropriate GIM_RecordInsn or GIM_CheckNumOperands has occurred before the hoisted predicate. At the moment, we're only managing to hoist the GIM_CheckOpcode for InsnID=0 which is always safe so we're getting away with it for now.
> > > 
> > > Here's roughly how I think we should go about fixing this:
> > > * We should prepare the way for GroupMatcher by changing the following:
> > >   * RuleMatcher should have a PredicateListMatcher
> > >   * Feature bit tests should be handled by a PredicateMatcher in the RuleMatcher
> > >   * InstructionMatcher and OperandMatcher should not have a PredicateListMatcher. Everything that was there should move to the RuleMatcher except for InstructionOperandMatcher's which should become a simple member of the OperandMatcher
> > > * Then we should introduce the GroupMatcher which should have an array of InstructionMatchers (just like RuleMatcher) to define the structure, and a PredicateListMatcher to define the conditions. We should distribute rules into groups based on the shape of the InstructionMatcher/OperandMatcher structure while preserving priority order. After grouping them, RuleMatcher should not hold any representation of structure.
> > > * Finally, for each nested GroupMatcher we should do what this code currently does
> > > 
> > > The overall effect will be that the top-level GIM_Try's will define the structure and emit the relevant GIM_RecordInsn's and GIM_CheckNumOperands. Deeper levels of GIM_Try's will only test conditions. Later on, we can then start breaking up the structure level into finer grain groupings and/or improve the intelligence of the grouping of the condition levels.
> > > 
> > > That said, it would be good to make some progress towards saner tables quickly. We could just adapt this code to intentionally hoist only the opcode for InsnID == 0 and improve things from there.
> > Given the record "directives" are not explicitly part of the predicate, I don't think we can mess them up. Indeed, we won't be able to hoist or reorder (which we don't do anyway) them, thus we cannot end up with a non-recorded use, right?
> > 
> > In other words, I don't think we need to restrict ourselves to InsnID == 0.
> > Am I missing something?
> The plan makes sense to me. It matches what I have in mind when I said we should flat the hierarchy.
Suppose you've hoisted a couple checks and have some rules in a group of the form:
  GIM_Try,
    GIM_CheckOpcode, /*InsnID*/0, G_ADD
    GIM_Try
      GIM_RecordInsn, /*InsnID*/1, ...
      GIM_CheckOpcode, /*InsnID*/1, G_MUL
      ...
    GIM_Try
      GIM_RecordInsn, /*InsnID*/1, ...
      GIM_CheckOpcode, /*InsnID*/1, G_MUL
      ...

Both rules have the same leading check, so that check is also hoisted by this code to get:
  GIM_Try,
    GIM_CheckOpcode, /*InsnID*/0, G_ADD
    GIM_CheckOpcode, /*InsnID*/1, G_MUL    // <-- InsnID 1 has not been defined by a GIM_RecordInsn yet
    GIM_Try
      GIM_RecordInsn, /*InsnID*/1, ... // <-- Was emitted by emitCaptureOpcodes()
        ...
    GIM_Try
      GIM_RecordInsn, /*InsnID*/1, ...
      ...

At the moment the only thing that seems to prevent this is that all the rules differ in the second predicate so it doesn't check whether the GIM_CheckOpcode for InsnID 1 can be hoisted.


Repository:
  rL LLVM

https://reviews.llvm.org/D39034