[PATCH] D36862: AMDGPU: Handle non-temporal loads and stores

[Tony Tye via Phabricator via llvm-commits]

t-tye added inline comments.


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:336-337
+  if (MOI.IsNonTemporal) {
+    // FIXME: handle non-temporal atomic loads?
+    assert(!MOI.IsAtomic);
+
----------------
b-sumner wrote:
> t-tye wrote:
> > According to http://llvm.org/docs/LangRef.html#load-instruction :
> > ```
> > !nontemporal does not have any defined semantics for atomic loads.
> > ```
> > 
> > My reading of this is that it is allowed to have nontemporal on atomic loads, but the meaning of it is not defined by LLVM. If so an assert is not correct here.
> > 
> > My suggestion is that AMDGPU target treats it the same as non-atomic.
> I'd prefer ignoring the non-temporal in the case of atomics.  Note that one would have to resort to coding in IR to even get it applied; no languages have a means to request non-temporal atomic operations.
So another patch should update the LLVM IR documentation. The syntax for load/store atomic already does not list nontemprarl as an allowed attribute, but the text mentions it. And also add a validator test to enforce that non-temporal is not allowed on load/store/rmw atomic.


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:64
+              AtomicOrdering FailureOrdering,
+              bool IsAtomic = true,
+              bool IsNonTemporal = false)
----------------
Is this needed? Isn't it duplicating the information from AtomicOrdering != NotAtomic?


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:65
+              bool IsAtomic = true,
+              bool IsNonTemporal = false)
         : SSID(SSID),
----------------
Is having a default value a good thing? The only use seems to be for fences so why not simply pass in false?


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:77
+          IsAtomic(MMO->isAtomic()),
+          IsNonTemporal(MMO->getFlags() & MachineMemOperand::MONonTemporal) {}
   };
----------------
assert(Ordering != NotAtomic || IsNonTemporal)?


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:254-255
     return None;
   if (!MI->hasOneMemOperand())
     return MemOpInfo();
 
----------------
What are examples of instructions that have multiple memory operands? Is is valid to treat them as system scope sequentially consistent atomic and !nontemporarl? Would it be clearer to call the constructor with the arguments explicit so can see what this is actually returning?

Same comment for other places doing this.


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:335
   bool Changed = false;
+  if (!MOI.IsAtomic && MOI.IsNonTemporal) {
+    Changed |= enableSLCBit(MI);
----------------
Should this be:


```
if (MOI.IsNonTemporal) {
  assert(!MOI.IsAtomic);
```

Since above comments are that nontemporal is not allowed on atomic.


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:342
+  // Must be atomic beyond this point.
+  assert(MOI.IsAtomic);
   if (MOI.SSID == SyncScope::System ||
----------------
Is there really a need for IsAtomic()? Isn't AtomicOrdering != NotAtomic giving the same information?


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:374
   bool Changed = false;
+  if (!MOI.IsAtomic && MOI.IsNonTemporal) {
+    Changed |= enableSLCBit(MI);
----------------
Same comment as above.


https://reviews.llvm.org/D36862