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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:1576-1577
+                                      TRI)) {
+      unsigned Width =
+          !MI.memoperands_empty() ? MI.memoperands().front()->getSize() : 0;
+      MemOpRecords.push_back(MemOpInfo(SU, BaseOps, Offset, Width));
----------------
This won't correctly handle multiple mem operands


================
Comment at: llvm/lib/Target/AMDGPU/SIInsertHardClauses.cpp:159-166
+        unsigned WidthA = CI.Last
+                              ? !CI.Last->memoperands_empty()
+                                    ? CI.Last->memoperands().front()->getSize()
+                                    : 0
+                              : 0;
+        unsigned WidthB =
+            !MI.memoperands_empty() ? MI.memoperands().front()->getSize() : 0;
----------------
hsmhsm wrote:
> arsenm wrote:
> > It would be better to not depend on the memory operands here, but this belongs in a helper function some kind of not (and this can also sink down to the use)
> Hi @arsenm 
> 
> Did you mean here the helper function as a kind of below?
> 
> ```
> unsigned getDstMemOperandSize(const MachineInstr *MI) const {
>   if (!MI || MI->memoperands_empty())
>     return 0;
> 
>   return MI->memoperands().front()->getSize();
> }
> ```
> 
> And, use above helper function as below?
> 
> 
> ```
> unsigned WidthA = getDstMemOperandSize(CI.Last);
> unsigned WidthB = getDstMemOperandSize(&MI);
> ```
> 
Yes, but it's worse to rely on the memory operands here than getting this from the instruction opcode / operand


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80545