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

Quentin Colombet via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat Dec 2 08:54:47 PST 2017 

qcolombet added a comment.

I'll upload a new version on Monday.



================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:594-597
+  void clearImplicitMap() {
+    NextInsnVarID = 0;
+    InsnVariableIDs.clear();
+  };
----------------
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!)


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:533
+  }
+  bool conditionMatches(const PredicateMatcher &Predicate) const;
+  bool conditions_empty() const { return Conditions.empty(); }
----------------
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 :)


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:700
 
-    for (const auto &Predicate : predicates())
+    unsigned OpIdx = (*predicates().begin())->getOpIdx();
+    for (const auto &Predicate : predicates()) {
----------------
dsanders wrote:
> predicates().begin() could be predicate_begin() which is more direct. Also this will need to be guarded against release builds otherwise some of the bots will object to the unused variable
Both good catches!


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:719-731
+    IPM_Opcode,
+    IPM_ImmPredicate,
+    IPM_NonAtomicMMO,
     OPM_Tie,
     OPM_ComplexPattern,
     OPM_IntrinsicID,
     OPM_Instruction,
----------------
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.)


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:2338
+          &Target.getInstruction(RK.getDef("G_CONSTANT")),
+          InsnMatcher.getVarID());
     } else
----------------
dsanders wrote:
> I think we should have InstructionMatcher::addPredicate forward its ID onto the predicates that are added to it
Good point!
I made the change mechanically, I didn't notice the pattern.



================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:2390
+      OM.addPredicate<LLTOperandMatcher>(
+          MemTyOrNone.getValue(), InsnMatcher.getVarID(), OM.getOperandIndex());
       continue;
----------------
dsanders wrote:
> Similarly, OperandMatcher::addPredicate should forward the Instruction ID and Operand Index to the predicates added to it
Yep!


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:3052
 
+std::vector<Matcher *> GlobalISelEmitter::OptimizeRules(
+    std::vector<RuleMatcher> &Rules,
----------------
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?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:3052
 
+std::vector<Matcher *> GlobalISelEmitter::OptimizeRules(
+    std::vector<RuleMatcher> &Rules,
----------------
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.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:3054
+    std::vector<RuleMatcher> &Rules,
+    std::vector<std::unique_ptr<GroupMatcher>> &StorageGroupMacther) {
+  std::vector<Matcher *> OptRules;
----------------
dsanders wrote:
> StorageGroupMacther -> StorageGroupMatcher
Good catch.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:3365-3367
+    // If this operand is free of constraints, rip it off.
+    if (OpMatcher.predicates_empty()) {
+      Matcher.pop_front();
----------------
dsanders wrote:
> This doesn't look right. It's removing the whole instruction matcher if its first operand matcher runs out of predicates. There may be other operand matchers left which do have predicates.
Good catch, this check is supposed to check that OpMatcher was the last one!


Repository:
  rL LLVM

https://reviews.llvm.org/D39034

More information about the llvm-commits mailing list