[PATCH] D40578: AMDGPU: Make hazard recognizer aware of maximum clause sizes

[Tony Tye via Phabricator via llvm-commits]

t-tye added inline comments.


================
Comment at: lib/Target/AMDGPU/GCNHazardRecognizer.cpp:374
+  if (ClauseSize > MaxClauseSize)
+    return 0;
+
----------------
arsenm wrote:
> t-tye wrote:
> > Would it be better to explicitly break the clause, as a context save restore may bring a wave back in mid clause execution and so the counters may saturate at a different instruction. By breaking the clause explicitly this will not be an issue.
> I don't understand what the issue is here. The counters stop working correctly?
When a context save happens the wave could be in the middle of a clause. When the context restore happens it behaves as if the clause started from the point of the context save (since all previous memory operations are waited to complete before the context save, and so the outstanding waitcnt counters start at 0 on the context restore). So the counters may saturate further down the clause than this code predicted.

XNACK replay can cause a similar situation since during replay the clause is being effectively started further in.

So my suggestion is that if the code is only scanning over a certain list of instructions to ensure the clause rules, it should explicitly break the clause at the point it scans up to. Otherwise a context save/restore or XNACK replay could effectively execute a clause that spans over instructions that have not been checked for hazards.

In practice it seems unlikely that clauses will reach the hardware counter limit, so this will likely not result in reduced performance. However, it ensures correctness in the unlikely case that they do.

So the counters are still working correctly, they just may resume from 0 mid clause if the hardware does a context save/restore or XNACK replay which implicitly waits for the outstanding memory operations to complete.


https://reviews.llvm.org/D40578