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

Daniel Sanders via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Dec 1 17:27:31 PST 2017


dsanders added inline comments.


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


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


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:700
 
-    for (const auto &Predicate : predicates())
+    unsigned OpIdx = (*predicates().begin())->getOpIdx();
+    for (const auto &Predicate : predicates()) {
----------------
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


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


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:2338
+          &Target.getInstruction(RK.getDef("G_CONSTANT")),
+          InsnMatcher.getVarID());
     } else
----------------
I think we should have InstructionMatcher::addPredicate forward its ID onto the predicates that are added to it


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


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


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


================
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();
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D39034





More information about the llvm-commits mailing list