[PATCH] D79792: [AMDGPU] New SIInsertHardClauses pass

Wed May 13 08:05:28 PDT 2020

arsenm added inline comments.

================
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);
----------------
foad wrote:
> foad wrote:
> > rampitec wrote:
> > > 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.
> > I don't know. I haven't noticed any problems in testing. I guess it just relies on running late enough in the pipeline.
> Would bundling actually fix the problem? What about other code that modifies bundles, like GCNHazardRecognizer::insertNoopInBundle? It feels like there's an arms race where one side invents mechanisms like bundling to stop instructions being inserted, and the other side goes and inserts instructions in the bundles anyway.
> 
> Having said that, I do have some patches in progress to remove GCNHazardRecognizer::insertNoopInBundle. But I don't know if there are any other places where bundles could be modified.
I think the hazard recognizer is the only place with a potentially legitimate reason to try inserting anything else in a bundle

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79792