This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Fix ATOMIC_CMP_SWAP_WITH_SUCCESS expansion
AbandonedPublic

Authored by uweigand on Sep 23 2017, 11:39 AM.

Details

Reviewers
t.p.northover
Summary

When expanding a ATOMIC_CMP_SWAP_WITH_SUCCESS, ExpandNode creates a comparison of the compare-and-swap result against the expected value. If the type is not legal, those values may need to be extended before actually doing the compare. This code seems incorrect. The compare-and-swap result is extended correctly, but the expected isn't in all case.

The problem occurs only if TLI.getExtendForAtomicOps returns either ISD::ZERO_EXTEND or ISD::ANY_EXTEND. In both cases, the current code does:

RHS = DAG.getNode(ISD::ZERO_EXTEND, dl, OuterType, Node->getOperand(2));

As far as I can tell, this should always be a no-op, since the type of Node->getOperand(2) is already equal OuterType. This is because the operands were already promoted by LegalizeIntegerTypes before we get there. But this promotion is just an "any-extend", it doesn't actually perform either zero- or sign-extension. Since the above line is also a no-op, the extension is then not performed at all. I have a test case where this causes the comparison to erroneously return failure.

I think ExpandNode should use an in-reg promotion from AtomicType at this point, to ensure that the value is actually extended correctly. This fixed my test case.

Doing this change causes two ARM test cases to fail since an additional extend instruction is now generated. In one place, I think this is actually necessary, and the code would have been incorrect without. In the second place, the extend instruction is redundant since the ARM back-end already generates one in ARMExpandPseudo::ExpandCMP_SWAP, but this is unknown to the SelectionDAG. This could likely be improved, but the source already says "This only gets used at -O0 so we don't care about efficiency of the generated code.", so this may not be important.

Diff Detail

Event Timeline

uweigand created this revision.Sep 23 2017, 11:39 AM

Note that as of r314428 I've switched the SystemZ back-end to use a custom expander for ATOMIC_CMP_SWAP_WITH_SUCCESS, which makes use of the condition code value set by the compare-and-swap hardware instruction to completely omit any extra comparison.

The common-code expander is no longer used at all on SystemZ, therefore this bug in that code no longer really matters to us. I guess it should still be fixed, in case other platforms run into the issue ...