llvm dev - Jun 2017 - Verifying Backend Schedule (Over)Coverage

I ran into an interesting problem when helping to land a scheduler .td file
that my colleague had written. The problem that came up was that a 
multiply/add pair was not combined into an madd, but just for our CPU. Upon
digging into it, the problem turned out to be that '(instregex
"^SUB" ...'
was matching "SUBREG_TO_REG" and incorrectly increasing the schedule
length.
I removed the overly aggressive match, but I noticed that there were lots
of instructions that matched multiple regex patterns in InstegexOp::apply in
utils/TableGen/CodeGenSchedule. I modified the apply method to output 
<idx, pat> pairs and <idx, name> pairs and then joined them togather
using a script. However, I couldn't easily determine from within that method
what specific subtarget the patterns came from. Is there a better place to do
this check? It seems that CodeGenSchedModels::checkCompleteness would be the
logical place.

Joel Jones

+cc Matthias, who has worked on the machine model generator more recently and
currently owns it.
> On Jun 21, 2017, at 2:08 PM, Joel Jones via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> I ran into an interesting problem when helping to land a scheduler .td file
> that my colleague had written. The problem that came up was that a 
> multiply/add pair was not combined into an madd, but just for our CPU. Upon
> digging into it, the problem turned out to be that '(instregex
"^SUB" ...'
> was matching "SUBREG_TO_REG" and incorrectly increasing the
schedule length.
> I removed the overly aggressive match, but I noticed that there were lots
> of instructions that matched multiple regex patterns in InstegexOp::apply
in
> utils/TableGen/CodeGenSchedule. I modified the apply method to output 
> <idx, pat> pairs and <idx, name> pairs and then joined them
togather
> using a script. However, I couldn't easily determine from within that
method
> what specific subtarget the patterns came from. Is there a better place to
do
> this check? It seems that CodeGenSchedModels::checkCompleteness would be
the
> logical place.
> 
> Joel Jones

Hi Joel,

As a general reminder, it's crucial to check the output from
-debug-only=subtarget-emitter after changing the machine model to make
sure it matches expectations. I'm afraid many people don't do that
because it's awkward and not at all discoverable.

If there's anyway you can improve on the presentation, diagnostics, and
debugging
of the generated model would be very helpful.

We should be catching overcoverage somewhere--I know that was the intention.

There are at least a few issues that makes all this confusing:

1. All subtargets of an architecture are "compressed" into a single
   table of MCWriteProcResEntries. That compression is done on-the-fly
   during tablegen subtarget emission.

2. An architecture can list default read/write resources for
   instruction classes, which are only overriden by the subtarget for
   selected opcodes.

3. Some scheduling classes are inferred based on custom "predicates"
   so they only apply to certain subtargets.

4. We still support itinerary classes defined on instruction
   definitions. That was done to continue supporting Cortex A9
   itineraries while simultaneously testing the new machine model on
   that target. If we could drop support for itinerary classes, it
   seems that a lot of complexity could be ripped out.

It's hard to say without running some experiments myself, but it looks
to me like the code in SubtargetEmitter::GenSchedClassTables just
picks the first subtarget-specific match for a given scheduling class:

        if (&ProcModel == &SchedModels.getProcModel(RWModelDef)) {
          RWDef = RW;
          break;
        }

It's strange to me that we don't assert here that there's only one
match.

Even if we add this assert, I agree that this should be diagnosed
first in checkCompleteness, before we ever start emitting tables. But
it should not be limited to "complete" machine models.

-Andy

Hi Joel,

devising some way to warn users when an instruction is matched by multiple
regexes would be great. As Andrew outlined below the trick is to identify
exceptional cases in which we should either not warn or give the user a way to
override the warning. Unfortunately I cannot give you much advice right now on
how to implement it best, off hand but would also need to dive back into the
code. I'd be happy to review patches!

You probably have seen llvm/utils/schedcover.py which is a neat tool to get some
overview of the scheduling models of a target. Not sure whether it helps in your
specific case though (it depends on whether we get debug prints for every regex
match or just for the one that is picked in the end).

- Matthias
> On Jun 26, 2017, at 4:40 PM, Andrew Trick via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> +cc Matthias, who has worked on the machine model generator more recently
and currently owns it.
> 
>> On Jun 21, 2017, at 2:08 PM, Joel Jones via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
>> 
>> I ran into an interesting problem when helping to land a scheduler .td
file
>> that my colleague had written. The problem that came up was that a 
>> multiply/add pair was not combined into an madd, but just for our CPU.
Upon
>> digging into it, the problem turned out to be that '(instregex
"^SUB" ...'
>> was matching "SUBREG_TO_REG" and incorrectly increasing the
schedule length.
>> I removed the overly aggressive match, but I noticed that there were
lots
>> of instructions that matched multiple regex patterns in
InstegexOp::apply in
>> utils/TableGen/CodeGenSchedule. I modified the apply method to output 
>> <idx, pat> pairs and <idx, name> pairs and then joined them
togather
>> using a script. However, I couldn't easily determine from within
that method
>> what specific subtarget the patterns came from. Is there a better place
to do
>> this check? It seems that CodeGenSchedModels::checkCompleteness would
be the
>> logical place.
>> 
>> Joel Jones
> 
> 
> Hi Joel,
> 
> As a general reminder, it's crucial to check the output from
> -debug-only=subtarget-emitter after changing the machine model to make
> sure it matches expectations. I'm afraid many people don't do that
> because it's awkward and not at all discoverable.
> 
> If there's anyway you can improve on the presentation, diagnostics, and
debugging
> of the generated model would be very helpful.
> 
> We should be catching overcoverage somewhere--I know that was the
intention.
> 
> There are at least a few issues that makes all this confusing:
> 
> 1. All subtargets of an architecture are "compressed" into a
single
>   table of MCWriteProcResEntries. That compression is done on-the-fly
>   during tablegen subtarget emission.
> 
> 2. An architecture can list default read/write resources for
>   instruction classes, which are only overriden by the subtarget for
>   selected opcodes.
> 
> 3. Some scheduling classes are inferred based on custom
"predicates"
>   so they only apply to certain subtargets.
> 
> 4. We still support itinerary classes defined on instruction
>   definitions. That was done to continue supporting Cortex A9
>   itineraries while simultaneously testing the new machine model on
>   that target. If we could drop support for itinerary classes, it
>   seems that a lot of complexity could be ripped out.
> 
> It's hard to say without running some experiments myself, but it looks
> to me like the code in SubtargetEmitter::GenSchedClassTables just
> picks the first subtarget-specific match for a given scheduling class:
> 
>        if (&ProcModel == &SchedModels.getProcModel(RWModelDef)) {
>          RWDef = RW;
>          break;
>        }
> 
> It's strange to me that we don't assert here that there's only
one match.
> 
> Even if we add this assert, I agree that this should be diagnosed
> first in checkCompleteness, before we ever start emitting tables. But
> it should not be limited to "complete" machine models.
> 
> -Andy
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170627/b6ec96e3/attachment.html>