This is an archive of the discontinued LLVM Phabricator instance.

[PR27599] [SystemZ] [SelectionDAG] Fix extension of atomic cmpxchg result.
ClosedPublic

Authored by koriakin on May 2 2016, 4:21 AM.

Details

Summary

Currently, SelectionDAG assumes 8/16-bit cmpxchg returns either a sign
extended result, or a zero extended result. SystemZ takes a third
option by returning junk in the high bits (rotated contents of the other
bytes in the memory word). In that case, don't use Assert*ext, and
zero-extend the result ourselves if a comparison is needed.

Diff Detail

Repository
rL LLVM

Event Timeline

koriakin retitled this revision from to [PR27599] [SystemZ] [SelectionDAG] Fix extension of atomic cmpxchg result..
koriakin updated this object.
koriakin added reviewers: uweigand, resistor.
koriakin set the repository for this revision to rL LLVM.
koriakin added a subscriber: llvm-commits.
dsanders added inline comments.May 10 2016, 5:03 AM
lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2863

ISD::ZERO_EXTEND only takes one operand so the DAG.getValueType(AtomicType) argument isn't necessary.

2871

I'm not sure why the ANY_EXTEND case returns ExtRes.getValue(0) instead of LHS.getValue(0). Is there a reason for it?

I'm currently thinking that returning LHS allows SelectionDAG to take advantage of the additional known-zero bits from the zero extend but returning ExtRes allows more scheduling freedom because it can be used before the comparison completes. I'm not sure which is the better choice though.

koriakin added inline comments.May 10 2016, 5:21 AM
lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2871

The comparison result may be unused and DCEd - in this case, it's better not to do the zero extension at all.

Fixed inline comment.

koriakin marked an inline comment as done.May 10 2016, 5:30 AM
dsanders accepted this revision.May 10 2016, 7:42 AM
dsanders added a reviewer: dsanders.

LGTM with the tests changes.

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2871

That makes sense to me. ATOMIC_CMP_SWAP_WITH_SUCCESS is always a strong operation so it's not strange to ignore the success result.

test/CodeGen/SystemZ/cmpxchg-05.ll
5–11

This should check that the zero extend and comparison is not emitted in this test (and likewise below).

I think there should also be a test that doesn't ignore the success result to check the comparison is as expected and also one that zero extends the success result to check that multiple zero extends don't occur.

This revision is now accepted and ready to land.May 10 2016, 7:42 AM
This revision was automatically updated to reflect the committed changes.