This change is currently added to the Phabricator as a demonstration of direction; I figure it's easier to see the plan if there's some code to go along with it.
We are trying to move toward incorporating the element unordered-atomic memory intrinsics into the MemIntrinsic class hierarchy as follows:
- MemIntrinsic
- MemSetInst
- ElementUnorderedAtomicMemSetInst+
- MemSetInst
- MemTransferInst
- MemCpyInst
- ElementUnorderedAtomicMemCpyInst+
- MemMoveInst
- ElementUnorderedAtomicMemMoveInst+
- MemCpyInst
+ = not currently in the hierarchy, will be added in future.
Doing this addition in a single large step, for any of the element unordered-atomic memory intrinsics, would be a far-reaching change touching the code and/or tests for at least 30 separate passes -- everywhere that the memory intrinsic is referenced. As such it would be challenging to review; it would require diverse sets of familiarity with many passes to fully sign off on such a change.
Thus, the proposed plan is to, instead:
- Land changes for all three of the element unordered-atomic memory intrinsics (memcpy is done, and memset/memmove are in the pipe)
- Land a change that will add an isElementAtomic() method to MemIntrinsic that will return false (https://reviews.llvm.org/D34629) by default, and will return true only if the intrinsic called is one of the element unordered-atomic memory intrinsics.
- Land a separate change for each affected pass to prepare for the insertion of the element unordered-atomic memory intrinsics into the class hierarchy that: (a) inserts checks for MemIntrinsic::isElementAtomic() to act appropriately, and (b) inserts tests that will check for behaviour of the pass on the element unordered-atomic memory intrinsics. The expectation is that many of these tests will fail once an intrinsic is added to the class hierarchy -- these tests will act as a canary to ensure that passes & tests are properly updated when the memory intrinsic is added to the class hierarchy.
- Land a separate change for each memory intrinsic that will add it to the MemIntrinsic class hierarchy, and updates tests/passes as required to ensure proper functioning.
Note: One could argue that I could do this entire process for the existing element unordered-atomic memcpy as a proof of concept, and not add the element unordered-atomic memset/memmove intrinsics to LLVM until that is done. I think that's a way to go, but it would entail a roughly 3x increase in the number of changes that would need to be reviewed in step (3); as such, I would prefer to add the memset/memmove intrinsics, and then do all of the isElementAtomic() checks and test-canaries on each pass for all three intrinsics at once.
This transform isn't legal for atomic memset in general. (You have to check alignment, and whether the atomic load is supported.)