[PATCH] D80946: [AMDGPU/MemOpsCluster] Code clean-up around accessing of memory operand width

Mon Jun 1 19:30:17 PDT 2020

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

================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.h:836

+  unsigned getOpSize(const MachineInstr &MI, const MachineOperand &MO) const {
+    const TargetRegisterClass *DstRC = RI.getRegClassForReg(
----------------
arsenm wrote:
> Losing the units in the name is a regression. I would also pass in MRI explicitly rather than getting it from the parent function
The function getOperandSizeInBytes() is not something existing for quite some time, It was just added by me in my previous patch https://reviews.llvm.org/D80545.  Then, I saw few already existing functions with the name  `getOpSize` to get operand size in bytes (see ones just above this function).  So, I favored uniformity here, and added another required overloaded function here. In that since it is not a regression.

If we do not get MRI from the parent here, then, caller of the getOpSize() has to do it. Is not it good to get it one place rather calling getParents repeatedly from caller side in multiple places?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80946