This is an archive of the discontinued LLVM Phabricator instance.

[X86] Make `shouldExpandLogicAtomicRMWInIR` able to match both operands.
ClosedPublic

Authored by goldstein.w.n on Jan 19 2023, 4:52 PM.

Details

Summary

Previous logic was buggy and erroneously asserted that I->operand(0) must
be the RMW instruction. This change fixes that and makes it so that the
RMW instruction can be used in operand 0 or 1.

Also update the tests to explicitly test RMW as operand 0/1 (no change
to codegen).

Diff Detail

Event Timeline

goldstein.w.n created this revision.Jan 19 2023, 4:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 4:52 PM
goldstein.w.n requested review of this revision.Jan 19 2023, 4:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 4:52 PM

Make this fix complete

goldstein.w.n retitled this revision from [X86] In `shouldExpandLogicAtomicRMWInIR` return `CmpXChg` instead of asserting if we don't match op(0) to [X86] Make `shouldExpandLogicAtomicRMWInIR` able to match both operands..Jan 19 2023, 9:01 PM
goldstein.w.n edited the summary of this revision. (Show Details)
goldstein.w.n edited the summary of this revision. (Show Details)
goldstein.w.n edited the summary of this revision. (Show Details)

Fix commit message

llvm/test/CodeGen/X86/atomic-rm-bit-test.ll
159

This seems to change RA (or maybe something else). Whatever it is there is some logic that is a bit faulty and make assumption about operands.

Also note, the new tests are able to reproduce the assertion bug.

pengfei accepted this revision.Jan 19 2023, 11:41 PM

LGTM with some nits.

llvm/lib/Target/X86/X86ISelLowering.cpp
31534–31536

Do you have a test for it? I suspect it's dead code. Maybe just use assert?

31539–31540

This can trun into auto *C1 = cast<ConstantInt>(AI->getValOperand());.

31608–31609

ditto.

This revision is now accepted and ready to land.Jan 19 2023, 11:41 PM
goldstein.w.n marked an inline comment as done.

dyn_cast -> cast. Add test for dead and

Correct align in test

llvm/lib/Target/X86/X86ISelLowering.cpp
31534–31536

Do you have a test for it? I suspect it's dead code. Maybe just use assert?

I think assert is a mistake b.c its definitely representable in IR:

define zeroext i16 @atomic_shl1_small_mask_xor_16_gpr_val(ptr %v, i16 zeroext %c) nounwind {
entry:
  %0 = and i16 %c, 7
  %shl = shl nuw nsw i16 1, %0
  %1 = atomicrmw xor ptr %v, i16 %shl monotonic, align 2
  %and = and i16 %1, %1
  ret i16 %and
}

I think its okay to miss optimizations when the IR is funky
but we shouldn't crash.

Added a test just to make sure we don't crash.

31539–31540

This can trun into auto *C1 = cast<ConstantInt>(AI->getValOperand());.

RKSimon added inline comments.Jan 20 2023, 8:56 AM
llvm/test/CodeGen/X86/atomic-rm-bit-test-64.ll
1474

I'd expect DAG canonicalization to commute this in DAGCombiner::visitAND?

goldstein.w.n added inline comments.Jan 20 2023, 10:55 AM
llvm/test/CodeGen/X86/atomic-rm-bit-test-64.ll
1474

I'd expect DAG canonicalization to commute this in DAGCombiner::visitAND?

The version that had assertions for canonical constant/non-constant operands did hit them so I expect there is some sequence where we go to shouldExpandLogicAtomicRMWInIR before canonicalization.