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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:11954-11955
+        auto SSID = RMW->getSyncScopeID();
+        if (SSID == SyncScope::System ||
+            SSID == RMW->getContext().getOrInsertSyncScopeID("one-as"))
+          return AtomicExpansionKind::CmpXChg;
----------------
t-tye wrote:
> rampitec wrote:
> > rampitec wrote:
> > > rampitec wrote:
> > > > t-tye wrote:
> > > > > rampitec wrote:
> > > > > > rampitec wrote:
> > > > > > > t-tye wrote:
> > > > > > > > t-tye wrote:
> > > > > > > > > rampitec wrote:
> > > > > > > > > > rampitec wrote:
> > > > > > > > > > > t-tye wrote:
> > > > > > > > > > > > t-tye wrote:
> > > > > > > > > > > > > rampitec wrote:
> > > > > > > > > > > > > > arsenm wrote:
> > > > > > > > > > > > > > > rampitec wrote:
> > > > > > > > > > > > > > > > arsenm wrote:
> > > > > > > > > > > > > > > > > rampitec wrote:
> > > > > > > > > > > > > > > > > > arsenm wrote:
> > > > > > > > > > > > > > > > > > > Isn't there some scope greater or equal function? Should avoid directly referencing one-as
> > > > > > > > > > > > > > > > > > It is, but it is located in the MMI whcih is not yet available here. In fact I don't even have MF yet to query it.
> > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > Anyway since it is specific to the only target and that target does not have a higher scope I suppose it shall be OK.
> > > > > > > > > > > > > > > > > Can it be moved out of the MMI? That sounds like a weird place for it
> > > > > > > > > > > > > > > > Any good ideas where to? It needs an LLVMContext, so definitely something which has access to Module.
> > > > > > > > > > > > > > > It seems like its a target specific context, its own thing separate from MMI. I guess we don't have anything like that now, but I would think it would look like a function returning a struct initialized on the first call
> > > > > > > > > > > > > > Technically it depends on the target (scope ordering) and not on a subtarget. At least so far and at least so far we do not envision so drastic changes that would require a subtarget differentiation of the scopes. I guess we would resist as much as we can if that is proposed. So technically it should not need MF, only the Module.
> > > > > > > > > > > > > This needs to happen regardless of scope. We implement these operations as rmw atomic instructions regardless of the scope requested, so they will always be forwarded to the L2. If the memory address happens to have an MTYPE that causes them not to happen in the L2, then the expansion must happen. Since the compiler does not know what memory may be being accessed the expansion must happen for all scopes for all accesses.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Unsafe-atomics can relax this for scopes <=agent. It is a promise that such atomics will never be to memory that will not be cached in the L2.
> > > > > > > > > > > > Scopes are target specific. Whether they are hierarchical is target specific. I believe there are target functions to query if the target supports scope inclusion, and will compare them. The SIMemoryLegalizer is using them to determine the memory properties of atomic instructions.
> > > > > > > > > > > That's exactly what's written in the code. If unsafe atomics are not enabled it will bail to CAS two lines before. What exactly do you want to change here?
> > > > > > > > > > "scopes are target specific". And? What exactly in the statement they "are target specific" precludes them from being target specific?
> > > > > > > > > Sounded like a question was being asked about scopes being hierarchical so just pointing to SIMemoryLegalizer that does use that concept. But reading the code more closely I see that that is not relevant here.
> > > > > > > > Sorry, I misread the code so my comment can be ignored.
> > > > > > > > 
> > > > > > > > But I did have a question about why the address space is being considered in deciding if expansion is needed.
> > > > > > > It is more about if it is target specific, or subtarget specific. So far it is not subtarget specific, I hope it will stay that way.
> > > > > > That because of the ISA. If we don't have needed instructions we need to expand it. Compare exchange is universally available, but specific instructions only in some address spaces.
> > > > > So cmpxchg is only available for FLAT address space? So what happens for GLOBAL address space, doesn't that have the same issue?
> > > > Most of the cases end up with cmpxchg, which is universally available in isa and safe. As I said, stars have to align for someone to get it in a form different from a CAS loop.
> > > Comment is ignored, granted. What about "revision needs change" status?
> > In general I think all of this discussion about avoiding directly accessing "one-as" roots into the the issue that our scopes are strings and not standard symbolic enums and all the attached frustration. I completely agree, but this is really much bigger than this w/a.
> > What about "revision needs change" status?
> 
> I tried to remove the "revision needs change" status, did I succeed?
Yep, thank you!


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

https://reviews.llvm.org/D98085