This is an archive of the discontinued LLVM Phabricator instance.

[DAG Legalization] Zero-extend the value compared in cmpxchg
AbandonedPublic

Authored by nemanjai on Jan 5 2018, 3:49 PM.

Details

Summary

Current legalization scheme extends input operands to atomic compare and exchange without specifying whether it's a sign or zero extension. However, this leads to bugs at least in PPC codegen. Of course, this can be fixed in the PPC back end, but it seems like the right thing to do would be to change this in legalization since the underlying comparison is an equality comparison. Operands of equality comparisons (SETCC) are zero-extended as well.

Since this affects other back ends as well, I'll send out an RFC about this to make sure this is safe/desirable for other targets.

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai created this revision.Jan 5 2018, 3:49 PM
efriedma added inline comments.
lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
236

The value operand to a store is implicitly truncated. The same thing happens for ATOMIC_LOAD_*. The same thing happens for the third operand of ATOMIC_CMP_SWAP. I think it's more confusing than helpful to add a special case for the compare operand of a compare. Atomic compare-and-swap doesn't zero-extend on all targets, anyway; see getExtendForAtomicOps().

(It looks like https://reviews.llvm.org/D41798 fixes your testcase?)

(It looks like https://reviews.llvm.org/D41798 fixes your testcase?)

Almost. I've added a detailed comment there regarding why it doesn't fix the whole problem.

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
236

I agree that this seems a bit confusing - i.e. why is one operand zero-extended whereas the other operand is essentially "any-extended". However, I think that the "store" operand of the atomic compare and exchange should be extended according to getExtendForAtomicOps() whereas the "compare" operand should always be zero-extended.
The reason I think this is that the compare operand is essentially used for equality comparison similarly to SETEQ/SETNE for SETCC, whereas the other operand is used to load and store the value - so it should be handled the way a target desires to handle atomics.

nemanjai abandoned this revision.Jan 8 2018, 10:00 AM

Abandoning this as at least one target prefers that the compare input be any-extended and each target decide what to do about the high bits that may differ between the two values.

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
236

I had planned to guard this zero-extension with TLI.getExtendForAtomicOps() == ISD::ZERO_EXTEND. However, since there are objections to doing this in legalization (see D41798), I'll abandon this and fix it in the PPC back end.