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

Sat May 30 06:20:26 PDT 2020

hsmhsm marked an inline comment as done.
hsmhsm added inline comments.

================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:302
+  const MachineOperand *DstOp = getDstMemOperand(LdSt);
+  Width = DstOp ? getDstOpSizeInBytes(LdSt, DstOp) : 0;
+
----------------
hsmhsm wrote:
> hsmhsm wrote:
> > 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 :)
> DstOp is null in many unit test-cases. I am not sure, what is going-on here. I need to debug it. It may take some time. I am not sure, if it is even worth considering the current situation and other P1 tickets at my hand. Anyway, let me see what best I can do here.
Hi @foad 

Only above review comment to be taken care in the patch, and all other review comments are taken care. However, since we will not be going to use the `width` value until we will be able to come-up with a safe heuristic for mem ops cluster threshold, we can safely merge this patch, and in the next patch, we can address this review comment. That way, we no need to block this entire big patch to be in hold condition, and we do not need to deal with merge conflicts often and often. What do you say? 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80545