[PATCH] D114351: [AMDGPU] Add SIMemoryLegalizer comments to clarify bit usage (NFC)

[Tony Tye via Phabricator via llvm-commits]

t-tye added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp:878
 
     return Changed;
   }
----------------
critson wrote:
> foad wrote:
> > Not really related to your patch, but why do we return here? Doesn't that mean that IsNonTemporal is effectively ignored if IsVolatile is true? Wouldn't it be both better and simpler to fall through to the IsNonTemporal handling here?
> This is a good point.  From a bit setting perspective it would be fine.
> Of course there is the question of what it semantically means to have a volatile nontemporal access when we seem to define volatile as "bypasses all caches".
I suspect this is because at one time relaxed atomics were marked as volatile. This may have been because the C/C++/OpenCL standards defined them that way, or because LLVM back then did not fully support atomics so marking them all as volatile made the existing passes "do the right thing". So this code may have been an attempt not to pessimize normal relaxed atomics. LLVM does not support non-temporal atomics currently.

I am not sure if these reasons are still the case so would be good to investigate and potentially fix this code (or at least document why it is the way it is with a FIXME). That can be a separate review I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114351