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

[Jay Foad via Phabricator via llvm-commits]

foad 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)
----------------
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.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:302
+  const MachineOperand *DstOp = getDstMemOperand(LdSt);
+  Width = DstOp ? getDstOpSizeInBytes(LdSt, DstOp) : 0;
+
----------------
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.


================
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();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80545