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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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.
----------------
t-tye wrote:
> 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.
This was already handled, the clause is broken if it's a store


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


https://reviews.llvm.org/D40578