[PATCH] D79792: [AMDGPU] New SIInsertHardClauses pass

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIInsertHardClauses.cpp:118
+            TII->get(AMDGPU::S_CLAUSE))
+        .addImm(std::min(CI.Length, 64u) - 1);
+    return true;
----------------
foad wrote:
> rampitec wrote:
> > Just break the scan at 64 and restart. Also needed test for this.
> OK, but my way was simpler.
> 
> I tried to add a test (hard-clauses.mir) but this pass is guided by the SIInstrInfo::shouldClusterMemOps heuristic, which never clusters that many loads. I still think this pass should handle it correctly, in case the heuristic ever changes.
This way you will restart new clause after the break, so it is better.

We may also want to add an option to shouldClusterMemOps() controlling a maximum cluster for this test. This option will be useful anyway for clustering experiments.


================
Comment at: llvm/lib/Target/AMDGPU/SIInsertHardClauses.cpp:116-118
+    BuildMI(*CI.First->getParent(), *CI.First, CI.First->getDebugLoc(),
+            TII->get(AMDGPU::S_CLAUSE))
+        .addImm(CI.Length - 1);
----------------
arsenm wrote:
> Do we need to bundle the claused instructions? What prevents inserting something else between these?
Looking at the passes which work after this it does not seem anything will be modified here. But you are probably right, just to be on a safer side.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79792