[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:366-368
   // We need to make sure not to put loads and stores in the same clause if they
   // use the same address. For now, just start a new clause whenever we see a
   // store.
----------------
We must also not put stores in the same clause if they may write the same address. Putting stores in their own clause will trivially satisfy this requirement.

How does this code ensure multiple consecutive stores are not put in the same clause? Seems it will only break the clause if a read is before a store. How is a store before a read broken into a separate clause?

This code seems to be per single basic block? What ensures that a clause cannot straddle across basic blocks (due to fall through)? Since no checking is being done across basic blocks the conservative action is to break clauses that end a basic block.


================
Comment at: lib/Target/AMDGPU/GCNHazardRecognizer.cpp:374
+  if (ClauseSize > MaxClauseSize)
+    return 0;
+
----------------
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.


https://reviews.llvm.org/D40578