[PATCH] D37397: AMDGPU: Handle more than one memory operand in SIMemoryLegalizer

[Konstantin Zhuravlyov via Phabricator via llvm-commits]

kzhuravl added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUMachineModuleInfo.h:51
+
+    return uint8_t(-1);
+  }
----------------
t-tye wrote:
> Would be better to ``reportUnknownSynchScope(MI);`` since this code would need to be updated if the target introduced another sync scope value.
Discussed a different approach offline.


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:204
+  AtomicOrdering FailureOrdering = AtomicOrdering::NotAtomic;
+
+  for (const auto &MMO : MI->memoperands()) {
----------------
t-tye wrote:
> Should there be a TODO saying that this logic does not check that if MMOs are present they cover the entire set of locations accessed by the memory instruction. If they only partially cover then would need to assume the conservative assumption of sequentially consistent system scope.
Discussed offline: this should be in validator. I have put a comment.


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:227-230
+  // Be conservative if there are no memory operands.
+  if (MI->getNumMemOperands() == 0)
     return SIMemOpInfo(SyncScope::System,
                        AtomicOrdering::SequentiallyConsistent);
----------------
t-tye wrote:
> This logic seems to belong in ``constructFromMI`` which claims to create a SIMemOpInfo from a machine instruction. So it ought to do that for any machine instruction, including those with no MMOs.
> 
> Same comment for other cases.
Discussed offline.


https://reviews.llvm.org/D37397