[llvm] r291640 - Re-commit r289955: [X86] Fold (setcc (cmp (atomic_load_add x, -C) C), COND) to (setcc (LADD x, -C), COND) (PR31367)
Hans Wennborg via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 17 11:40:58 PST 2017
Thanks!
I understand the problem now but I probably don't have time to fix it today.
I've merged the revert to the branch in r292243.
On Tue, Jan 17, 2017 at 11:31 AM, Bob Wilson <bob.wilson at apple.com> wrote:
> OK, thanks. I reverted it in r292242.
>
> Can the revert be merged to the 4.0 branch?
>
>> On Jan 16, 2017, at 7:45 PM, Hans Wennborg <hans at chromium.org> wrote:
>>
>> Hi Bob,
>>
>> Very sorry about the breakage. I'm not at work today, but if it fixes
>> the breakage, do go ahead and revert my patch. I'll do it when I get
>> into the office tomorrow otherwise.
>>
>> Thanks,
>> Hans
>>
>> On Mon, Jan 16, 2017 at 7:37 PM, Bob Wilson via llvm-commits
>> <llvm-commits at lists.llvm.org> wrote:
>>> Here’s an example. For the attached file, the expected code is something like this:
>>>
>>> movl $2, %eax
>>> lock xaddl %eax, 12(%rdi)
>>> cmpl $-2, %eax
>>> jae LBB0_3
>>>
>>> With r291640, the comparison gets broken:
>>>
>>> lock addl $2, 12(%rdi)
>>> jae LBB0_3
>>>
>>> If the fix isn’t quick, can you revert this? It’s also broken on the 4.0 release branch.
>>>
>>>
>>>
>>>> On Jan 16, 2017, at 11:26 AM, Bob Wilson <bob.wilson at apple.com> wrote:
>>>>
>>>> Even with the fix from r291630, this still causes problems. I get widespread assertion failures in the Swift runtime's WeakRefCount::increment() function (in Swift’s stdlib/public/SwiftShims/RefCount.h file):
>>>>
>>>> void increment() {
>>>> uint32_t newval = __atomic_add_fetch(&refCount, RC_ONE, __ATOMIC_RELAXED);
>>>> assert(newval >= RC_ONE && "weak refcount overflow");
>>>> (void)newval;
>>>> }
>>>>
>>>> I don’t yet have a reduced test case or a description of why it fails, but reverting this change fixes the problem.
>>>>
>>>>> On Jan 10, 2017, at 5:36 PM, Hans Wennborg via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>>>>
>>>>> Author: hans
>>>>> Date: Tue Jan 10 19:36:57 2017
>>>>> New Revision: 291640
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=291640&view=rev
>>>>> Log:
>>>>> Re-commit r289955: [X86] Fold (setcc (cmp (atomic_load_add x, -C) C), COND) to (setcc (LADD x, -C), COND) (PR31367)
>>>>>
>>>>> This was reverted because it would miscompile code where the cmp had
>>>>> multiple uses. That was due to a deficiency in the existing code, which
>>>>> was fixed in r291630 (see the PR for details).
>>>>>
>>>>> This re-commit includes an extra test for the kind of code that got
>>>>> miscompiled: @test_sub_1_setcc_jcc.
>>>>>
>>>>> Modified:
>>>>> llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
>>>>> llvm/trunk/test/CodeGen/X86/atomic-eflags-reuse.ll
>>>>>
>>>>> Modified: llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.cpp?rev=291640&r1=291639&r2=291640&view=diff
>>>>> ==============================================================================
>>>>> --- llvm/trunk/lib/Target/X86/X86ISelLowering.cpp (original)
>>>>> +++ llvm/trunk/lib/Target/X86/X86ISelLowering.cpp Tue Jan 10 19:36:57 2017
>>>>> @@ -29391,11 +29391,19 @@ static SDValue combineSelect(SDNode *N,
>>>>> return SDValue();
>>>>> }
>>>>>
>>>>> -/// Combine:
>>>>> +/// Combine brcond/cmov/setcc/.. based on comparing the result of
>>>>> +/// atomic_load_add to use EFLAGS produced by the addition
>>>>> +/// directly if possible. For example:
>>>>> +///
>>>>> +/// (setcc (cmp (atomic_load_add x, -C) C), COND_E)
>>>>> +/// becomes:
>>>>> +/// (setcc (LADD x, -C), COND_E)
>>>>> +///
>>>>> +/// and
>>>>> /// (brcond/cmov/setcc .., (cmp (atomic_load_add x, 1), 0), COND_S)
>>>>> -/// to:
>>>>> +/// becomes:
>>>>> /// (brcond/cmov/setcc .., (LADD x, 1), COND_LE)
>>>>> -/// i.e., reusing the EFLAGS produced by the LOCKed instruction.
>>>>> +///
>>>>> /// Note that this is only legal for some op/cc combinations.
>>>>> static SDValue combineSetCCAtomicArith(SDValue Cmp, X86::CondCode &CC,
>>>>> SelectionDAG &DAG) {
>>>>> @@ -29410,7 +29418,7 @@ static SDValue combineSetCCAtomicArith(S
>>>>> if (!Cmp.hasOneUse())
>>>>> return SDValue();
>>>>>
>>>>> - // This only applies to variations of the common case:
>>>>> + // This applies to variations of the common case:
>>>>> // (icmp slt x, 0) -> (icmp sle (add x, 1), 0)
>>>>> // (icmp sge x, 0) -> (icmp sgt (add x, 1), 0)
>>>>> // (icmp sle x, 0) -> (icmp slt (sub x, 1), 0)
>>>>> @@ -29429,8 +29437,9 @@ static SDValue combineSetCCAtomicArith(S
>>>>> return SDValue();
>>>>>
>>>>> auto *CmpRHSC = dyn_cast<ConstantSDNode>(CmpRHS);
>>>>> - if (!CmpRHSC || CmpRHSC->getZExtValue() != 0)
>>>>> + if (!CmpRHSC)
>>>>> return SDValue();
>>>>> + APInt Comparand = CmpRHSC->getAPIntValue();
>>>>>
>>>>> const unsigned Opc = CmpLHS.getOpcode();
>>>>>
>>>>> @@ -29446,16 +29455,19 @@ static SDValue combineSetCCAtomicArith(S
>>>>> if (Opc == ISD::ATOMIC_LOAD_SUB)
>>>>> Addend = -Addend;
>>>>>
>>>>> - if (CC == X86::COND_S && Addend == 1)
>>>>> + if (Comparand == -Addend) {
>>>>> + // No change to CC.
>>>>> + } else if (CC == X86::COND_S && Comparand == 0 && Addend == 1) {
>>>>> CC = X86::COND_LE;
>>>>> - else if (CC == X86::COND_NS && Addend == 1)
>>>>> + } else if (CC == X86::COND_NS && Comparand == 0 && Addend == 1) {
>>>>> CC = X86::COND_G;
>>>>> - else if (CC == X86::COND_G && Addend == -1)
>>>>> + } else if (CC == X86::COND_G && Comparand == 0 && Addend == -1) {
>>>>> CC = X86::COND_GE;
>>>>> - else if (CC == X86::COND_LE && Addend == -1)
>>>>> + } else if (CC == X86::COND_LE && Comparand == 0 && Addend == -1) {
>>>>> CC = X86::COND_L;
>>>>> - else
>>>>> + } else {
>>>>> return SDValue();
>>>>> + }
>>>>>
>>>>> SDValue LockOp = lowerAtomicArithWithLOCK(CmpLHS, DAG);
>>>>> DAG.ReplaceAllUsesOfValueWith(CmpLHS.getValue(0),
>>>>>
>>>>> Modified: llvm/trunk/test/CodeGen/X86/atomic-eflags-reuse.ll
>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/atomic-eflags-reuse.ll?rev=291640&r1=291639&r2=291640&view=diff
>>>>> ==============================================================================
>>>>> --- llvm/trunk/test/CodeGen/X86/atomic-eflags-reuse.ll (original)
>>>>> +++ llvm/trunk/test/CodeGen/X86/atomic-eflags-reuse.ll Tue Jan 10 19:36:57 2017
>>>>> @@ -192,4 +192,68 @@ entry:
>>>>> ret i8 %s2
>>>>> }
>>>>>
>>>>> +define i8 @test_sub_1_setcc_eq(i64* %p) #0 {
>>>>> +; CHECK-LABEL: test_sub_1_setcc_eq:
>>>>> +; CHECK: # BB#0: # %entry
>>>>> +; CHECK-NEXT: lock decq (%rdi)
>>>>> +; CHECK-NEXT: sete %al
>>>>> +; CHECK-NEXT: retq
>>>>> +entry:
>>>>> + %tmp0 = atomicrmw sub i64* %p, i64 1 seq_cst
>>>>> + %tmp1 = icmp eq i64 %tmp0, 1
>>>>> + %tmp2 = zext i1 %tmp1 to i8
>>>>> + ret i8 %tmp2
>>>>> +}
>>>>> +
>>>>> +define i8 @test_add_5_setcc_ne(i64* %p) #0 {
>>>>> +; CHECK-LABEL: test_add_5_setcc_ne:
>>>>> +; CHECK: # BB#0: # %entry
>>>>> +; CHECK-NEXT: lock addq $5, (%rdi)
>>>>> +; CHECK-NEXT: setne %al
>>>>> +; CHECK-NEXT: retq
>>>>> +entry:
>>>>> + %tmp0 = atomicrmw add i64* %p, i64 5 seq_cst
>>>>> + %tmp1 = icmp ne i64 %tmp0, -5
>>>>> + %tmp2 = zext i1 %tmp1 to i8
>>>>> + ret i8 %tmp2
>>>>> +}
>>>>> +
>>>>> +define i8 @test_add_5_setcc_ne_comparand_mismatch(i64* %p) #0 {
>>>>> +; CHECK-LABEL: test_add_5_setcc_ne_comparand_mismatch:
>>>>> +; CHECK: # BB#0: # %entry
>>>>> +; CHECK-NEXT: movl $5, %eax
>>>>> +; CHECK-NEXT: lock xaddq %rax, (%rdi)
>>>>> +; CHECK-NEXT: testq %rax, %rax
>>>>> +; CHECK-NEXT: setne %al
>>>>> +; CHECK-NEXT: retq
>>>>> +entry:
>>>>> + %tmp0 = atomicrmw add i64* %p, i64 5 seq_cst
>>>>> + %tmp1 = icmp ne i64 %tmp0, 0
>>>>> + %tmp2 = zext i1 %tmp1 to i8
>>>>> + ret i8 %tmp2
>>>>> +}
>>>>> +
>>>>> +declare void @g()
>>>>> +define zeroext i1 @test_sub_1_setcc_jcc(i64* %p) local_unnamed_addr #0 {
>>>>> +; TODO: It's possible to use "lock dec" here, but both uses of the cmp need to
>>>>> +; be updated.
>>>>> +; CHECK-LABEL: test_sub_1_setcc_jcc:
>>>>> +; CHECK: # BB#0: # %entry
>>>>> +; CHECK: movq $-1, %rax
>>>>> +; CHECK-NEXT: lock xaddq %rax, (%rdi)
>>>>> +; CHECK-NEXT: cmpq $1, %rax
>>>>> +; CHECK-NEXT: sete %bl
>>>>> +; CHECK-NEXT: jne
>>>>> +entry:
>>>>> + %add = atomicrmw volatile add i64* %p, i64 -1 seq_cst
>>>>> + %cmp = icmp ne i64 %add, 1
>>>>> + %not = xor i1 %cmp, true
>>>>> + br i1 %cmp, label %else, label %then
>>>>> +then:
>>>>> + tail call void @g()
>>>>> + br label %else
>>>>> +else:
>>>>> + ret i1 %not
>>>>> +}
>>>>> +
>>>>> attributes #0 = { nounwind }
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>
More information about the llvm-commits
mailing list