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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:856
+      if (!TII->isMAI(MAI) ||
+          MAI.getOpcode() == AMDGPU::V_ACCVGPR_WRITE_B32_e64 ||
+          MAI.getOpcode() == AMDGPU::V_ACCVGPR_READ_B32_e64)
----------------
kerbowa wrote:
> jrbyrnes wrote:
> > rampitec wrote:
> > > kerbowa wrote:
> > > > arsenm wrote:
> > > > > What about copies before they are lowered to accvgpr_write?
> > > > I guess isMAI above would handle that?
> > > They are not MAI. This ideom used to filter MFMA from 2 other MAI encoded instructions.
> > Thanks for your comments. I looked into it, and these types of copies are not flagged as MAI to begin with -- the check to exclude accvgpr_write is thus irrelevant.
> I'm not sure, I think it will be flagged. We use this idiom enough that we really should just add an isMFMA function to siinstrinfo.
These 2 are also isMAI, just based on encoding. It may make sense to create a helper, agree.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:898
+        if (MFMAOpa->NodeNum > MFMAOpb->NodeNum)
+          std::swap(MFMAOpa, MFMAOpb);
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124678