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

[Mahesha S via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:298-299
+    const TargetRegisterInfo *TRI) const {
+  const MachineOperand *DstOp = getDstMemOperand(LdSt);
+  Width = DstOp ? getDstOpSizeInBytes(LdSt, DstOp) : 0;
+
----------------
hsmhsm wrote:
> foad wrote:
> > Move this below the mayLoadOrStore check. Then, does getDstMemOperand ever fail?
> Fixed it. However, I do not know much about the functionality guarantee of `mayLoadOrStore()`. So for the purpose of safety, I have added an `if stmt` to check if `DstOp` is `null or not`, and return `false` if it is `null`.
Apparently, above `if stmt` resulted in one unit test failure. Hence reverted it back. And again, assuming here that `DstOp` will not be `null` is also NOT guaranteed, We seem to be getting `null` in many unit test cases, and hence assuming it non-null will again result in many test failures. Now there two possibilities
(1)  `getDstMemOperand()` might not be handling all the cases OR
(2)  `getDstMemOperand()` is handling all the cases, but, it is expected that `DstOp` will be `null` in few cases even though, `mayLoadOrStore()` is true.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80545