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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

LGTM. We need to follow up with the refactor to eliminate clearImplicitMap() though. We ought to follow up with the simplification of PredicateListMatcher too. The templating isn’t very useful anymore now that predicates have a common base class



================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:594-597
+  void clearImplicitMap() {
+    NextInsnVarID = 0;
+    InsnVariableIDs.clear();
+  };
----------------
dsanders wrote:
> 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.
I dont think the comment has been added


================
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();
----------------
qcolombet wrote:
> qcolombet wrote:
> > 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!
> Actually, I've checked the implementation of InstructionMatcher::pop_front and it only does what I intended: removing the first operand.
> 
> Talking with Daniel offline there was something else to fix: getNumOperands, so that the related check remains correct (it was looking at the size of this list). 
Sorry about that.

There’s also the operand index which uses the position within this list


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:756
+
+  bool matchers_empty() const { return Matchers.empty(); }
+  void pop_front() { Matchers.erase(Matchers.begin()); }
----------------
Some of this predates this patch but we seem to have three naming conventions for operations affecting Matchers:
* insnmatchers_front
* matchers_empty
* pop_front
We should pick one, my preference is for the first two. Of those two I have a slight preference for insnmatchers_front


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:811
 
-    for (const auto &Predicate : predicates())
+    unsigned OpIdx = (*predicates_begin())->getOpIdx();
+    (void)OpIdx;
----------------
This will crash if predicates_begin() == predicates_end()


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:920
 public:
-  SameOperandMatcher(StringRef MatchingName)
-      : OperandPredicateMatcher(OPM_SameOperand), MatchingName(MatchingName) {}
+  SameOperandMatcher(StringRef TiedTo, unsigned InsnVarID, unsigned OpIdx)
+      : OperandPredicateMatcher(OPM_SameOperand, InsnVarID, OpIdx), TiedTo(TiedTo) {}
----------------
This seems to have undone the TiedTo -> MatchingName change I was asked to make when I committed SameOperandMatcher


Repository:
  rL LLVM

https://reviews.llvm.org/D39034