[PATCH] D68338: [AMDGPU] Remove dubious logic in bidirectional list scheduler

Thu Oct 3 06:06:01 PDT 2019

foad marked 2 inline comments as done.
foad added a comment.

In D68338#1691536 <https://reviews.llvm.org/D68338#1691536>, @rampitec wrote:

> I tend to agree with the reasoning. We can only judge after benchmarking though. You say you see more gains than loses. Can you elaborate on a magnitude of those?

I benchmarked on gfx10 with 324 shaders from 14 different games, which I don't think I can share publically.
10 of them slowed down by more than 2%. The worst slow-down was 8.5%.
38 of them sped up by more than 2%. The best speed-up was 45%.

In addition to the overall speed-up, my hope is that it will behave a bit more consistently, because it won't be able to get stuck in the odd state where a good candidate is consistently ignored.

In D68338#1691888 <https://reviews.llvm.org/D68338#1691888>, @atrick wrote:

> I guess this is just making the custom scheduling logic more closely match the generic scheduler? Makes sense to me.

Yes.

================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:234-236
-  if (TopCand.Reason == BotCand.Reason) {
-    Cand = BotCand;
-    GenericSchedulerBase::CandReason TopReason = TopCand.Reason;
----------------
arsenm wrote:
> The logic over here looks pretty convoluted to me
The old logic or the new logic? The new logic (like most of this function) is cut n pasted from the same function in GenericScheduler, and is dictated by the rather obscure API of tryCandidate. As I understand it, tryCandidate(Cand, TryCand) means "if TryCand is better than Cand then set TryCand.Reason to the reason why".

================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:245
-  } else {
-    if (TopCand.Reason == RegExcess && TopCand.RPDelta.Excess.getUnitInc() <= 0) {
-      Cand = TopCand;
----------------
@tstellar do you know or remember the reasons for all of this logic that looks at the RPDeltas?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68338/new/

https://reviews.llvm.org/D68338