[PATCH] D41798: [LegalizeDAG] Fix ATOMIC_CMP_SWAP_WITH_SUCCESS legalization.

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

I should start by saying that I think this patch seems to me rather uncontroversial, but I don't think it is adequate to address the original problem (explanation follows).

Shouldn't an equivalent thing be done for the ATOMIC_CMP_SWAP that is generated (since it will also have an underlying comparison)? What I see in the test case attached to the original patch is this:

  t19: i32,i1,ch = AtomicCmpSwapWithSuccess<Volatile LDST2[@Glob]> t2, GlobalAddress:i64<i16* @Glob> 0, Constant:i32<-3344>, Constant:i32<222>

becomes the following after operation legalization:

  t23: i32,ch = AtomicCmpSwap<Volatile LDST2[@Glob]> t2, t32, Constant:i32<-3344>, Constant:i32<222>
  t25: i32 = AssertZext t23, ValueType:ch:i16
  t29: i1 = setcc t25, Constant:i32<62192>, seteq:ch

So the operand to SETCC is zero-extended as desired, but the compare operand to the ATOMIC_CMP_SWAP is sign-extended (which in the test case I have included with the other patch will store the wrong value).

On the other hand, with the original patch the entire problem is resolved and I see:

  t6: i16,i1,ch = AtomicCmpSwapWithSuccess<Volatile LDST2[@Glob]> t2, GlobalAddress:i64<i16* @Glob> 0, Constant:i16<-3344>, Constant:i16<222>

becomes the following after type legalization:

  t21: i32,i1,ch = AtomicCmpSwapWithSuccess<Volatile LDST2[@Glob]> t2, GlobalAddress:i64<i16* @Glob> 0, Constant:i32<62192>, Constant:i32<222>

which will then be expanded into:

  t25: i32,ch = AtomicCmpSwap<Volatile LDST2[@Glob]> t2, t32, Constant:i32<62192>, Constant:i32<222>
  t27: i32 = AssertZext t25, ValueType:ch:i16
  t29: i1 = setcc t27, Constant:i32<62192>, seteq:ch

Namely, doing this in type legalization will zero-extend the compare operand for the node before we get to the expansion, so that the SETCC/ATOMIC_CMP_SWAP nodes generated for the expansion will have their compare operand zero-extended consistently. However, doing so would also zero-extend the compare operand to the ATOMIC_CMP_SWAP_WITH_SUCCESS node even if the operation is custom-legalized and although I don't imagine any target relies on this not happening, I suppose it's possible that some target is.

In any case, this patch does not sufficiently address the motivating issue for the original patch (from: https://bugs.llvm.org/show_bug.cgi?id=35812).


Repository:
  rL LLVM

https://reviews.llvm.org/D41798