[PATCH] D98085: [AMDGPU] Always expand system scope fp atomics on gfx90a

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In D98085#2612608 <https://reviews.llvm.org/D98085#2612608>, @rampitec wrote:

> In D98085#2609265 <https://reviews.llvm.org/D98085#2609265>, @rampitec wrote:
>
>> I am looking where to move syncscope initialization and cannot find a place. I do not think I can just initialize it on a first query and cache it because it depends on the LLVMContext. I have also double checked that MF is not created yet and MMI is not created yet too.
>
> Note what would happen if I cache this value at the first call:
>
>   SyncScope::ID LLVMContextImpl::getOrInsertSyncScopeID(StringRef SSN) {
>     auto NewSSID = SSC.size();
>     assert(NewSSID < std::numeric_limits<SyncScope::ID>::max() &&
>            "Hit the maximum number of synchronization scopes allowed!");
>     return SSC.insert(std::make_pair(SSN, SyncScope::ID(NewSSID))).first->second;
>   }
>
> It will lock numeric value depending on the call history of getOrInsertSyncScopeID(). Caching without mapping to a context will prohibit any part of the compiler to ever call getOrInsertSyncScopeID() and then reuse AMDGPU BE. If the same instance of SIISelLowering will be called with a different module with a different Context cache may contain a wrong value. Besides even that depends on the internal implementation of the Context. AMDGPUMachineModuleInfo is fine do it because it has module context, but anything before MF is created not.

I didn't mean the LLVMContext, I mean a new target context which would be a new concept. That doesn't necessarily belong in this patch, but I do think it would be better


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

https://reviews.llvm.org/D98085