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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:3052
 
+std::vector<Matcher *> GlobalISelEmitter::OptimizeRules(
+    std::vector<RuleMatcher> &Rules,
----------------
dsanders wrote:
> 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.
Good catch. I mis-remembered that record insn stuff was also emitted as part of the emitPredicates thing.
Do you think I should add a emitCaptureOpcodes as part of the GroupMatcher as well?

Anyhow, for now, I will just bail out on InsnID != 0 or use OpIdx.


Repository:
  rL LLVM

https://reviews.llvm.org/D39034