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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

> CmpVal: Should not need a LL[HC]R, as it should already be zero-extended also in the case of a non-constant, or?

Not necessarily.   Our ABI does require that "char" and "short" parameters and return values are extended, but that can be either a zero- or a sign-extension depending on the type.  Also, this is implemented via the zeroext/signext type attributes on the parameters in code generated by clang; with LLVM IR generated elsewhere (like in those test cases!), we may get a plain i8 or i16 that is not extended.  And of course if the i8 or i16 in question is not a function parameter but the result of some intermediate computation, it is not guaranteed to be extended anyway.

So in short, yes, the CmpVal may have to be extended.  However, it is probably worthwhile to detect those (common) cases where it already *is* extended to avoid redundant effort.  This is hard(er) to do at the MI level, so I think the extension is best done in SystemZTargetLowering::lowerATOMIC_CMP_SWAP at the SelectionDAG level before emitting the ATOMIC_CMP_SWAPW MI instruction.

As an aside, it seems the code does now require one extra register.  It might be worthwhile to avoid this by rearranging the statements a bit:

  //  LoopMBB:
  //   %OldVal        = phi [ %OrigOldVal, EntryBB ], [ %RetryOldVal, SetMBB ]
  //   %SwapVal       = phi [ %OrigSwapVal, EntryBB ], [ %RetrySwapVal, SetMBB ]
  //   %Dest          = RLL %OldVal, BitSize(%BitShift)
  //                      ^^ The low BitSize bits contain the field
  //                         of interest.
  //   %RetrySwapVal = RISBG32 %SwapVal, %Dest, 32, 63-BitSize, 0
  //                      ^^ Replace the upper 32-BitSize bits of the
  //                         swap value with those that we loaded and rotated.
  //   %Dest    = LL[CH] %Dest
  //   CR %Dest, %CmpVal
  
  //  SetMBB:
  //   %StoreVal     = RLL %RetrySwapVal, -BitSize(%NegBitShift)
  //                      ^^ Rotate the new field to its proper position.
  //   %RetryOldVal  = CS %OldVal, %StoreVal, Disp(%Base)
  //   JNE LoopMBB

As an added bonus, this would make Dest already zero-extended, so it might be possible to avoid any extra zero-extension on the result (by adding an AssertZExt ISD node after the ATOMIC_SWAP_CMPW).


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

https://reviews.llvm.org/D97604