[PATCH] D48190: [MCA][x86][NFC] Add tests for -register-file-stats, -scheduler-stats

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added inline comments.


================
Comment at: test/tools/llvm-mca/X86/register-file-statistics.s:45
+# SKLSER-NEXT:    Total number of mappings created:    2
+# SKLSER-NEXT:    Max number of mappings used:         2
+
----------------
lebedev.ri wrote:
> andreadb wrote:
> > lebedev.ri wrote:
> > > andreadb wrote:
> > > > lebedev.ri wrote:
> > > > > RKSimon wrote:
> > > > > > A lot of these prefixes look redundant?
> > > > > Oh, indeed.
> > > > > 
> > > > > BTW, in this file, everything before `Register File statistics:` isn't expected to be here.
> > > > > Can
> > > > > ```
> > > > > Iterations:        100
> > > > > Instructions:      100
> > > > > Total Cycles:      51
> > > > > Dispatch Width:    2
> > > > > IPC:               1.96
> > > > > Block RThroughput: 0.5
> > > > > ```
> > > > > be named a "-basic-stats", and an option to control that output be added?
> > > > > @andreadb does that sound reasonable?
> > > > Not really.
> > > > I think the summary view should never be optional in llvm-mca.
> > > > It is only a few lines, and it gives a nice overview of the run.
> > > > 
> > > I agree that it is quite essential for normal usage,
> > > but in the edge-case use-case like this, it does not seem to be of much use..
> > My opinion is that register pressure should be always put into perspective. 
> > Register pressure tends to change over the course of a simulation for many reasons (i.e. the presence of long latency instructions, long data dependencies, lack of resources versus high dispatch rate). So, the number of iterations usually matters, and it helps putting the register usage into perspective.
> > That being said, if other reviewers are okay with this change, then I won't oppose it.
> I'm not denying any of that.
> I'm simply saying that in **these two testcases**, the only thing they are intended to produce,
> is to dump the `ProcResGroup<>.BufferSize`, and `RegisterFile<>.NumPhysRegs`.
As I wrote, I am okay with this change if other reviewers are okay with it.
Since Clement (and presumably Greg) are okay with it (based on the feedback from D48209), then I am not going to oppose this patch.

As a side note, the subject of this patch is a bit misleading.
There are already existing tests for `-register-file-stats` and  `-scheduler-stats` in `X86/BtVer2` directory.

My understanding is that your original plan is to add test coverage specifically for the register file usage in Znver1 (in the summary, you wrote that this is a spin-off of D47676). If my understanding is correct, then please move these tests into directory ZnVer1, and make those tests znver1 only.


Repository:
  rL LLVM

https://reviews.llvm.org/D48190