[PATCH] D80545: [AMDGPU/MemOpsCluster] Let mem ops clustering logic also consider number of clustered bytes

[Mahesha S via Phabricator via llvm-commits]

hsmhsm marked 3 inline comments as done.
hsmhsm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:277-292
+const MachineOperand *
+SIInstrInfo::getDstMemOperand(const MachineInstr &LdSt) const {
+  const MachineOperand *DstOp = nullptr;
+
+  if (isMUBUF(LdSt) || isMTBUF(LdSt) || isFLAT(LdSt)) {
+    DstOp = getNamedOperand(LdSt, AMDGPU::OpName::vdata);
+    if (!DstOp)
----------------
foad wrote:
> I would prefer this logic to be folded into getMemOperandsWithOffsetWidth itself, where we already test for all the different instruction types (isMUBUF and so on). It might make sense for each of the cases in getMemOperandsWithOffsetWidth to set DstOp and then fall through to some common code at the end of the function that sets Width based on DstOp.
In fact, I initially thought about it. However, first look at the code within getMemOperandsWithOffsetWidth() made me to feel on the other way around. I felt that this function requires clean-up, and hence did not want to touch it. Anyway, let me follow your suggestion. I will experiment and prepare a patch.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:302
+  const MachineOperand *DstOp = getDstMemOperand(LdSt);
+  Width = DstOp ? getDstOpSizeInBytes(LdSt, DstOp) : 0;
+
----------------
foad wrote:
> Hopefully if you follow the advice above, you will find DstOp is never null (because it is only ever null here in cases where getMemOperandsWithOffsetWidth is going to return false anyway).
> 
> If you do still find cases where DstOp is null then please debug them to see what has gone wrong.
I will hope for the best :)


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:2775-2780
   if (!MIa.hasOneMemOperand() || !MIb.hasOneMemOperand()) {
     // FIXME: Handle ds_read2 / ds_write2.
     return false;
   }
   unsigned Width0 = MIa.memoperands().front()->getSize();
   unsigned Width1 = MIb.memoperands().front()->getSize();
----------------
foad wrote:
> This code can be removed now. You should be able to use the Width values returned by getMemOperandsWithOffsetWidth instead. But you can leave that for a later patch if you prefer.
I would prefer to make this change in a separate patch, after the current patch is successfully merged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80545