[PATCH] D21723: [RFC] Enhance synchscope representation

[Justin Lebar via llvm-commits]

jlebar added a comment.

Tony, thanks for your comments.

In https://reviews.llvm.org/D21723#548834, @tony-tye wrote:

> I would argue that even without (2) there is a case for using standard LLVM atomic instructions as it makes it possible to generate the same IR for a given language regardless of whether the target supports scopes.


OK.  Personally I don't find this particularly compelling, but I agree it's something.  And it sounds like you agree below that it's not the most important reason.

> Currently it appears that atomic machine instructions have to be marked as volatile to ensure that the machine code instruction scheduler will insert the necessary dependencies. This is conservatively correct, but if the memory machine instructions had the memory ordering information available it would be possible to only add dependencies in the direction required in a target neutral way. This does not require knowledge of the memory scope.


Sure, although this could also be solved by adding some sort of metadata to our intrinsic functions so that their operands are annotated correctly.  It seems to me that this would be desirable even if we were to add this support for scoped atomics, because it'd be useful for other intrinsics?

For example, LLVM IR doesn't appear to have a compare-and-swap instruction, and until one is added, we're stuck using target-specific intrinsics for those (right?).  But whatever volatility stuff you want to remove from atomic intrinsics, presumably you'd also want to remove from a CAS intrinsic?

If the above is right I don't find it a particularly compelling reason to go with this approach.  (That is, I come back to "letting us write generic IR optimizations that take advantage of target-specific information about their atomics.")

> If alias analysis was to use address space information, then it would seem it would need to query the target to determine if two address spaces may alias (for example, for OpenCL the generic address space may alias other address spaces, but other address spaces do not alias each other).


Target-specific AA may be instructive.  See https://reviews.llvm.org/D24441 and https://reviews.llvm.org/D12414, my attempt at adding an NVPTX-specific AA.  We could formulate this as "address spaces X and Y cannot alias", but it would be much less powerful than what I have in that patch.

> Currently the concept of memory scope already exists in LLVM as the SynchonizationScope enumeration, and is encoded in bit code as a 32 bit value.

> 

> If in writing future optimizations a better representation is devised then it could always be changed as a separate patch at that time.


I guess I feel like this kind of sidesteps the question, which is, would one of the representations I mooted (or some other representation) be better?  I acknowledge that the current SycnchScope enum is like address spaces, but as I wrote above, I'm not convinced the way we do target-specific address spaces is good, because it makes our IR very hard to read -- you as the human have to keep this mapping in your head.  It'll be worse in some way with atomics because they're uncommon, so you're less likely to have the mapping memorized.

Currently there are only two possible values for the synchscope, so maybe it's not so painful at the moment, but we're proposing adding a lot more.  As you say, there isn't a lot of code today that uses these values, so I guess I'm saying that changing now may be better than changing after we write a bunch of code that relies on the opaque numeric representation.  Or at least, I'd like us to think about it.

> Currently there are no uses of SynchonizationScope in any optimizations, even though there are two scopes currently defined. Are there plans for future atomic optimizations?


I think this gets to the heart of the matter; we should understand the use-cases for this metadata.


https://reviews.llvm.org/D21723