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

[Tony Tye via Phabricator via llvm-commits]

t-tye added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUMachineModuleInfo.h:38
 
+  /// \returns \p SSID's width.
+  uint8_t getSSIDWidth(SyncScope::ID SSID) const {
----------------
Suggest more descriptive comment:
```
/// \returns \p SSID's position in the total ordering of sync scopes such that a wider scope has a higher value than a narrower scope.
```


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


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:204
+  AtomicOrdering FailureOrdering = AtomicOrdering::NotAtomic;
+
+  for (const auto &MMO : MI->memoperands()) {
----------------
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.


================
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);
----------------
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.


https://reviews.llvm.org/D37397