[PATCH] D124678: [AMDGPU] Allow for MFMA Inst Clustering

[Jeffrey Byrnes via Phabricator via llvm-commits]

jrbyrnes added a comment.

In D124678#3482965 <https://reviews.llvm.org/D124678#3482965>, @rampitec wrote:

> One thing to mention: transferring all successors and predecessors technically does not guarantee that an independent instruction will not be scheduled in between. I see that you are adding SDep::Cluster, but I remember it didn't use to work in the very similar scenario before. Not sure it is still so. It can happen that in a bigger program the cluster may be broken.
>
> Also we probably need to consider dropping MFMA clustering along with load clustering during GCNScheduleDAGMILive::UnclusteredReschedule stage.

Hey Stas -- thanks for your comments.

Regarding your note -- yes this is something I spent some time thinking about – Sdep::Cluster doesn’t gaurantee a single cluster. In fact, I believe there is a hardware dependency between MFMA’s, so the scheduler will try to fill this gap with an independent instruction.

It seems that if we want to offer unbroken clusters, we will want to offer a scheduler with modified heuristic priorities to prioritize cluster edges. We can also address the issue of UnclusteredReschedule pass in this scheduler. Worth mentioning is that in such a scheme, a cluster can still be broken if the preds / succs of instructions are not handled, which is why I’ve handled them here.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:898
+        if (MFMAOpa->NodeNum > MFMAOpb->NodeNum)
+          std::swap(MFMAOpa, MFMAOpb);
+
----------------
rampitec wrote:
> kerbowa wrote:
> > rampitec wrote:
> > > What if one feeds another and you swap them?
> > This is only looking at independent MFMA.
> Ah, right. Interesting if we may want to cluster dependent MFMA too or not.
As the code currently is, an instruction that has dependency with any instruction in Cluster A will be used in Cluster B, and so on. We may be able to cluster instructions with dependencies, but this would create complexities. I thought it best to get the simple version for review first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124678