[PATCH] D120370: [AMDGPU] Fix combined MMO in load-store merge

[Jay Foad via Phabricator via llvm-commits]

foad accepted this revision.
foad added a comment.
This revision is now accepted and ready to land.

LGTM, thanks. Choosing the first MMO (in pointer order) makes sense to me, as then we don't have to update the offset or the alignment info, just the size.



================
Comment at: llvm/lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:683
+
+  // A base pointer for the combined operation is the same as a leading
+  // operation's pointer.
----------------
Typo "**the** leading operations's pointer".


================
Comment at: llvm/lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:665-666
 
-// This function assumes that \p A and \p B have are identical except for
+// This function assumes that \p A and \p B are identical except for
 // size and offset, and they reference adjacent memory.
 static MachineMemOperand *combineKnownAdjacentMMOs(MachineFunction &MF,
----------------
rampitec wrote:
> foad wrote:
> > If this comment was true ("A and B are identical except for size and offset") then your patch would not be necessary.
> In fact I think this comment is generally incorrect. I'd probably better remove it al all. This is the point of the patch, use the MMO of the leading operation, this is why I have changed one of the pointers in the test to float, to spot that.
> In fact I think this comment is generally incorrect. I'd probably better remove it al all.

Agreed.


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

https://reviews.llvm.org/D120370