[bdver2] Incorrect (2 instead of 1) store throughput

[LebedevRI]

Bugzilla Link	39465
Resolution	FIXED
Resolved on	Jun 19, 2019 01:52
Version	trunk
OS	Linux
Blocks	#31672
CC	@adibiagio,@topperc,@RKSimon
Fixed by commit(s)	346404, 346587

Extended Description

As discussed in https://reviews.llvm.org/D52779#1273934

[LebedevRI]

I have added the tests in https://reviews.llvm.org/rL346404

And i'm not sure i understand.

The two-loads-per cycle is clearly modelled correctly, as you can see.
But similarly, only one load per cycle is also modelled correctly.
Am i testing this wrong?

[adibiagio]

I have added the tests in https://reviews.llvm.org/rL346404

And i'm not sure i understand.

The two-loads-per cycle is clearly modelled correctly, as you can see.
But similarly, only one load per cycle is also modelled correctly.
Am i testing this wrong?

I think you are not doing anything wrong.

I forgot about the LoadStoreUnit (LSUnit). My bad...
Basically,
Even though there are two available units, the load/store unit forces the younger load to "wait" until the older store is executed. That is because of the rules enforced by the implemented consistency model, which prevents reordering of store operations.

Although technically people can define their own consistency model for the LSUnit, I think you are fine. Even if you don't change the model, you still cannot get anything more than 1 store per cycle. So, you should be fine.

The bottom line is: I don't think that you need to fix it, because the default memory consistency model implemented by the LSUnit will prevent two stores to be issued during the same cycle.

--

As a side note:

while looking into this, I noticed something very strange...
For some reasons, all your loads and stores seem to consume resources scheduler queue entries in bot PdLoad and PdStore. Regardless of whether the operation is a load or a store.

You can see this if you enable statistics for the scheduler usage.
Schedulers "PdLoad" and "PdStore" are always used together when a load or store is dispatched.

As an experiment, you can have a look at the scheduling description for MOV8mr in X86GenSubtargetInfo.inc. It looks like there are 3 write processor resources. It consumes:

• 1cy of PdEX
• 1cy of PdLoad (!)
• 1cy of PdStore

Same for the MOV8rm:

• 1cy of PdEX
• 1cy of PdLoad
• 1cy of PdStore (!)

Note that PdLoad and PdStore are essentially equivalent modulo the buffer size (which is 40 for PdLoad, and 24 for PdStore).

What I think is happening is:
the SubtargetEmitter sees a use for 1cy of resource PdAGLU01 (a resource with 2 units). It then searches for all the groups containing PdAGLU01. In this case, it finds that PdAGLU01 is contained by:

• PdEX
• PdLoad
• PdStore

So, it subtracts 1cy to all of them.

This is incorrect. I think it can be fixed if you really split the memory scheduler into two distinct schedulers.

Note that PdEx is currently declared as a unified scheduler which contains everything. Every load or store would always consume 1 entry in PdEx too. Is that intentional?

[adibiagio]

Sorry for the confusion (here is the correct first paragraph):

I forgot about the LoadStoreUnit (LSUnit). My bad...
Basically, 
Even though there are two available units, the load/store unit forces the younger STORE to "wait" until the older STORE is executed. That is because the implemented consistency model prevents from reordering store operations.

In conclusion:
there is a minor problem related to the scheduler queue consumption.

In particular, at the moment:

• a load consumes one entry in the load queue, and one entry in the store queue.
• same for the store...

[LebedevRI]

The bottom line is: I don't think that you need to fix it, because the
default memory consistency model implemented by the LSUnit will prevent two
stores to be issued during the same cycle.
OK, thank you for checking, makes sense.

As a side note:

...

This is incorrect. I think it can be fixed if you really split the memory
scheduler into two distinct schedulers.
Aha. I will look into that

Note that PdEx is currently declared as a unified scheduler which contains
everything. Every load or store would always consume 1 entry in PdEx too. Is
that intentional?

Yes, that is intentional.

Actually, i wonder if i'm undermodelling this.
E.g. https://www.realworldtech.com/bulldozer/6/ says that 128-entry retirement queue
should also keep track of both the PdEx and PdFPU macro-ops.
Since currently i only have the default
def PdRCU : RetireControlUnit<128, 4>;
does it already magically do that,
or are the macro-ops only consume an entry in PdRCU after they finish executing?
I'm thinking the latter.

[adibiagio]

Actually, i wonder if i'm undermodelling this.
E.g. https://www.realworldtech.com/bulldozer/6/ says that 128-entry
retirement queue
should also keep track of both the PdEx and PdFPU macro-ops.
Since currently i only have the default
def PdRCU : RetireControlUnit<128, 4>;
does it already magically do that,
or are the macro-ops only consume an entry in PdRCU after they finish
executing?
I'm thinking the latter.

Entries in the reorder buffer are consumed at dispatch time. A token is returned by the retire control unit. That token is eventually used to claim back resources once the instruction can be retired.

So, it should just magically do that :-).

[LebedevRI]

Actually, i wonder if i'm undermodelling this.
E.g. https://www.realworldtech.com/bulldozer/6/ says that 128-entry
retirement queue
should also keep track of both the PdEx and PdFPU macro-ops.
Since currently i only have the default
def PdRCU : RetireControlUnit<128, 4>;
does it already magically do that,
or are the macro-ops only consume an entry in PdRCU after they finish
executing?
I'm thinking the latter.

Entries in the reorder buffer are consumed at dispatch time. A token is
returned by the retire control unit. That token is eventually used to claim
back resources once the instruction can be retired.
I have come to the the same conclusion after posting the question, after reading
https://llvm.org/doxygen/MCSchedule_8h_source.html#l00271
https://llvm.org/docs/CommandGuide/llvm-mca.html#instruction-dispatch

So, it should just magically do that :-).
I guess the thing that confused me is that it is "implicit",
not displayed in e.g. scheduler stats.

[adibiagio]

At the moment, the -retire-stats view only prints a simple histogram of instructions retired per cycle.

It could be improved by adding extra information on the average and maximum number of ROB entries consumed over the entire simulation.

[LebedevRI]

This is incorrect. I think it can be fixed if you really split the memory
scheduler into two distinct schedulers.
Aha. I will look into that
Looked into that, and not quite sure yet.
There are two AGU pipes, and you can have either two loads,
or one load plus one store. Not two loads plus a store.

So abstractly, i would need to have two pipes (AGU0, AGU1),
and 3 resource units (Load0, Load1, Store), with grouping:

• Load0 -> AGU0
• Load1, Store -> AGU1
  Not sure yet how to achieve that while fixing the issue at hand
  (not consuming the store sched queue with load entries.)

[adibiagio]

Try this:

Index: X86ScheduleBdVer2.td

--- X86ScheduleBdVer2.td (revision 346487)
+++ X86ScheduleBdVer2.td (working copy)
@@ -130,15 +130,13 @@
// Load-Store Units
//

-// FIXME: does this even make sense?

-def PdLoad : ProcResGroup<[PdAGLU01]> {

• // For Piledriver, the load queue is 40 entries deep.
  +let Super = PdAGLU01 in
  +def PdLoad : ProcResource<2> {
  let BufferSize = 40;
  }

-def PdStore : ProcResGroup<[PdAGLU01]> {

• // For Piledriver, the store queue is 24 entries deep.
  +let Super = PdAGLU01 in
  +def PdStore : ProcResource<1> {
  let BufferSize = 24;
  }

I think it should work.
The downside is that you will have to regenerate a lot of tests because now the resources have changed.

As a side note: PdLoad and PdStore are treated like "scheduler" resources. So, any stalls caused by the lack of scheduler tokens are still reported by the "scheduler-stats" view. There is an open bugzilla which is about "marking resources that are clearly used to model load/store queues". I don't remember which one it is, but it should be one of those prefixed with [llvm-mca].

[adibiagio]

As a side note: PdLoad and PdStore are treated like "scheduler" resources.
So, any stalls caused by the lack of scheduler tokens are still reported by
the "scheduler-stats" view. There is an open bugzilla which is about
"marking resources that are clearly used to model load/store queues". I
don't remember which one it is, but it should be one of those prefixed with
[llvm-mca].

Sorry.
What I mean is:
if there is a stall caused by the lack of buffer resources in PdStore/PdLoad, the dispatch-stats view will classify it as a SCHEDULER stall.

The scheduler-stats view also treats PdLoad/PdStore as "scheduler resources"; that's why they will always appear in the scheduler-stats.

That upstream bugzilla I mentioned, is about tagging buffers that are not "scheduler resources", but instead describe load/store queues.

[adibiagio]

So, I tried that suggested patch. However, the "Super" is unfortunately treated specially by the SubtargetEmitter...
Basically, if a "super" resource is part of a group G, the "consumed resource cycles" for group G is not updated.

I never figured out whether that was a bug or not.
But that is how method SubtargetEmitter::ExpandProcResource() works in practice.

[adibiagio]

So, I tried that suggested patch. However, the "Super" is unfortunately
treated specially by the SubtargetEmitter...
Basically, if a "super" resource is part of a group G, the "consumed
resource cycles" for group G is not updated.

I never figured out whether that was a bug or not.
But that is how method SubtargetEmitter::ExpandProcResource() works in
practice.

With that patch, you get the right load/store queue consumption.
However, you lose the buffer update for scheduler PdEX. That is because SubtargetEmitter assumes that PdAGLU01 is consumed, but not PdEX...

[adibiagio]

So, I tried that suggested patch. However, the "Super" is unfortunately
treated specially by the SubtargetEmitter...
Basically, if a "super" resource is part of a group G, the "consumed
resource cycles" for group G is not updated.

I never figured out whether that was a bug or not.
But that is how method SubtargetEmitter::ExpandProcResource() works in
practice.

With that patch, you get the right load/store queue consumption.
However, you lose the buffer update for scheduler PdEX. That is because
SubtargetEmitter assumes that PdAGLU01 is consumed, but not PdEX...

On a second thought, SubtargetEmitter is probably doing the right thing, and the problem may be in llvm-mca instead.

By construction, a "super" resource cannot be a group. That constraint is imposed by method SubtargetEmitter::ExpandProcResource().

A super resource S, is allowed to be part of a group G. Method ExpandProcResource, will not recursively analyze G.
I thought that was a bug. In retrospect, that is okay, because - at the end of the day - we only care about the set of consumed processor resource units, and not necessarily groups.

So, we don't need to analyze G in the subtarget emitter. Instead, llvm-mca should do some extra bookkeping and check if G is also a buffered resource. In case, G has to be added as one of the buffers.

I have a patch for llvm-mca that fixes the issue. I want to run some more tests before committing it. So, it will probably happen on next monday.

[adibiagio]

http://llvm.org/viewvc/llvm-project?view=revision&revision=346545

If you now try the suggested patch from my Comment 9, you should be able to see that correct numbers are produced for buffered resources in all the tests.

I hope it helps!

[LebedevRI]

http://llvm.org/viewvc/llvm-project?view=revision&revision=346545

If you now try the suggested patch from my Comment 9, you should be able to
see that correct numbers are produced for buffered resources in all the
tests.

I hope it helps!

Thank you for such a speedy patch!
I will take a look..