[PATCH] D97604: [SystemZ] Reimplement the 1-byte compare-and-swap logic

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

In D97604#2596148 <https://reviews.llvm.org/D97604#2596148>, @jonpa wrote:

> I added an AND to zero-out the high bits to perform the zero-extension from the narrow type. It seemed that if the source was a constant (e.g. '1'), the DAG.getNode(ISD::AND, ...) call folded the AND on the fly. And if the source was a parameter with the 'zeroext' attribute (or rather any result with an AssertZext node) ,the AND goes away during DAGCombine2. So for what I could see, there is not much extra work to do here.

Ah, thanks for pointing out the TLI.getExtendForAtomicCmpSwapArg() and TLI.getExtendForAtomicOps() routines, I had overlooked those.  This means you don't have to do any of the extends "by hand", you just use the proper opcode in those routines.  The TLI.getExtendForAtomicCmpSwapArg() routine specifies how common code is supposed to extend the compare value before passing it to compare-and-swap, while the TLI.getExtendForAtomicOps() routine specifies what extension platform-specific code will have already performed on the result so common code may rely on it.

These are currently both set to ANY_EXTEND on SystemZ, I think for the new algorithm you can just set them both to ZERO_EXTEND and everything those just work.

> At this point I am wondering about how to treat the signedness of the input/output: if the template type of std::atomic is signed, then the result of e.g. a signed char should be sign-extended to i32, right? So both CmpVal and Dest should either be sign- or zero-extended. The patch currently always zero-extends...

Well, I guess there could be multiple variants (sign/zero-extend to i32/i64) that we might support.  This may need improvements to the common-code TIL.getExtendForAtomic... logic (or possibly be done via DAGCombine)?   In any case, I think for now we should just do the ZERO_EXTEND, any improvement can be done as a follow-up.


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

https://reviews.llvm.org/D97604