<div dir="ltr">Yes, 100% agreement.<div class="gmail_extra"><br><div class="gmail_quote">On Mon, Dec 19, 2016 at 11:30 AM, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" target="_blank" class="cremed">hans@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks! Sorry for the breakage.<br>
<br>
(fwiw, it would have been good to reply on the original commit or code<br>
review when reverting so those following along get notified)<br>
<div class="HOEnZb"><div class="h5"><br>
On Sun, Dec 18, 2016 at 6:36 AM, Daniel Jasper via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org" class="cremed">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: djasper<br>
> Date: Sun Dec 18 08:36:38 2016<br>
> New Revision: 290066<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=290066&view=rev" rel="noreferrer" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-<wbr>project?rev=290066&view=rev</a><br>
> Log:<br>
> Revert r289955 and r289962. This is causing lots of ASAN failures for us.<br>
><br>
> Not sure whether it causes and ASAN false positive or whether it<br>
> actually leads to incorrect code or whether it even exposes bad code.<br>
> Hans, I'll get you instructions to reproduce this.<br>
><br>
> Modified:<br>
> llvm/trunk/lib/Target/X86/<wbr>X86ISelLowering.cpp<br>
> llvm/trunk/test/CodeGen/X86/<wbr>atomic-eflags-reuse.ll<br>
><br>
> Modified: llvm/trunk/lib/Target/X86/<wbr>X86ISelLowering.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.cpp?rev=290066&r1=290065&r2=290066&view=diff" rel="noreferrer" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/Target/<wbr>X86/X86ISelLowering.cpp?rev=<wbr>290066&r1=290065&r2=290066&<wbr>view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/lib/Target/X86/<wbr>X86ISelLowering.cpp (original)<br>
> +++ llvm/trunk/lib/Target/X86/<wbr>X86ISelLowering.cpp Sun Dec 18 08:36:38 2016<br>
> @@ -28985,19 +28985,11 @@ static SDValue combineSelect(SDNode *N,<br>
> return SDValue();<br>
> }<br>
><br>
> -/// Combine brcond/cmov/setcc/.. based on comparing the result of<br>
> -/// atomic_load_add to use EFLAGS produced by the addition<br>
> -/// directly if possible. For example:<br>
> -///<br>
> -/// (setcc (cmp (atomic_load_add x, -C) C), COND_E)<br>
> -/// becomes:<br>
> -/// (setcc (LADD x, -C), COND_E)<br>
> -///<br>
> -/// and<br>
> +/// Combine:<br>
> /// (brcond/cmov/setcc .., (cmp (atomic_load_add x, 1), 0), COND_S)<br>
> -/// becomes:<br>
> +/// to:<br>
> /// (brcond/cmov/setcc .., (LADD x, 1), COND_LE)<br>
> -///<br>
> +/// i.e., reusing the EFLAGS produced by the LOCKed instruction.<br>
> /// Note that this is only legal for some op/cc combinations.<br>
> static SDValue combineSetCCAtomicArith(<wbr>SDValue Cmp, X86::CondCode &CC,<br>
> SelectionDAG &DAG) {<br>
> @@ -29006,7 +28998,7 @@ static SDValue combineSetCCAtomicArith(S<br>
> (Cmp.getOpcode() == X86ISD::SUB && !Cmp->hasAnyUseOfValue(0))))<br>
> return SDValue();<br>
><br>
> - // This applies to variations of the common case:<br>
> + // This only applies to variations of the common case:<br>
> // (icmp slt x, 0) -> (icmp sle (add x, 1), 0)<br>
> // (icmp sge x, 0) -> (icmp sgt (add x, 1), 0)<br>
> // (icmp sle x, 0) -> (icmp slt (sub x, 1), 0)<br>
> @@ -29025,9 +29017,8 @@ static SDValue combineSetCCAtomicArith(S<br>
> return SDValue();<br>
><br>
> auto *CmpRHSC = dyn_cast<ConstantSDNode>(<wbr>CmpRHS);<br>
> - if (!CmpRHSC)<br>
> + if (!CmpRHSC || CmpRHSC->getZExtValue() != 0)<br>
> return SDValue();<br>
> - APInt Comparand = CmpRHSC->getAPIntValue();<br>
><br>
> const unsigned Opc = CmpLHS.getOpcode();<br>
><br>
> @@ -29043,19 +29034,16 @@ static SDValue combineSetCCAtomicArith(S<br>
> if (Opc == ISD::ATOMIC_LOAD_SUB)<br>
> Addend = -Addend;<br>
><br>
> - if (Comparand == -Addend) {<br>
> - // No change to CC.<br>
> - } else if (CC == X86::COND_S && Comparand == 0 && Addend == 1) {<br>
> + if (CC == X86::COND_S && Addend == 1)<br>
> CC = X86::COND_LE;<br>
> - } else if (CC == X86::COND_NS && Comparand == 0 && Addend == 1) {<br>
> + else if (CC == X86::COND_NS && Addend == 1)<br>
> CC = X86::COND_G;<br>
> - } else if (CC == X86::COND_G && Comparand == 0 && Addend == -1) {<br>
> + else if (CC == X86::COND_G && Addend == -1)<br>
> CC = X86::COND_GE;<br>
> - } else if (CC == X86::COND_LE && Comparand == 0 && Addend == -1) {<br>
> + else if (CC == X86::COND_LE && Addend == -1)<br>
> CC = X86::COND_L;<br>
> - } else {<br>
> + else<br>
> return SDValue();<br>
> - }<br>
><br>
> SDValue LockOp = lowerAtomicArithWithLOCK(<wbr>CmpLHS, DAG);<br>
> DAG.ReplaceAllUsesOfValueWith(<wbr>CmpLHS.getValue(0),<br>
><br>
> Modified: llvm/trunk/test/CodeGen/X86/<wbr>atomic-eflags-reuse.ll<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/atomic-eflags-reuse.ll?rev=290066&r1=290065&r2=290066&view=diff" rel="noreferrer" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/test/<wbr>CodeGen/X86/atomic-eflags-<wbr>reuse.ll?rev=290066&r1=290065&<wbr>r2=290066&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/test/CodeGen/X86/<wbr>atomic-eflags-reuse.ll (original)<br>
> +++ llvm/trunk/test/CodeGen/X86/<wbr>atomic-eflags-reuse.ll Sun Dec 18 08:36:38 2016<br>
> @@ -176,45 +176,4 @@ entry:<br>
> ret i8 %tmp2<br>
> }<br>
><br>
> -define i8 @test_sub_1_setcc_eq(i64* %p) #0 {<br>
> -; CHECK-LABEL: test_sub_1_setcc_eq:<br>
> -; CHECK: # BB#0: # %entry<br>
> -; CHECK-NEXT: lock decq (%rdi)<br>
> -; CHECK-NEXT: sete %al<br>
> -; CHECK-NEXT: retq<br>
> -entry:<br>
> - %tmp0 = atomicrmw sub i64* %p, i64 1 seq_cst<br>
> - %tmp1 = icmp eq i64 %tmp0, 1<br>
> - %tmp2 = zext i1 %tmp1 to i8<br>
> - ret i8 %tmp2<br>
> -}<br>
> -<br>
> -define i8 @test_add_5_setcc_ne(i64* %p) #0 {<br>
> -; CHECK-LABEL: test_add_5_setcc_ne:<br>
> -; CHECK: # BB#0: # %entry<br>
> -; CHECK-NEXT: lock addq $5, (%rdi)<br>
> -; CHECK-NEXT: setne %al<br>
> -; CHECK-NEXT: retq<br>
> -entry:<br>
> - %tmp0 = atomicrmw add i64* %p, i64 5 seq_cst<br>
> - %tmp1 = icmp ne i64 %tmp0, -5<br>
> - %tmp2 = zext i1 %tmp1 to i8<br>
> - ret i8 %tmp2<br>
> -}<br>
> -<br>
> -define i8 @test_add_5_setcc_ne_<wbr>comparand_mismatch(i64* %p) #0 {<br>
> -; CHECK-LABEL: test_add_5_setcc_ne_comparand_<wbr>mismatch:<br>
> -; CHECK: # BB#0: # %entry<br>
> -; CHECK-NEXT: movl $5, %eax<br>
> -; CHECK-NEXT: lock xaddq %rax, (%rdi)<br>
> -; CHECK-NEXT: testq %rax, %rax<br>
> -; CHECK-NEXT: setne %al<br>
> -; CHECK-NEXT: retq<br>
> -entry:<br>
> - %tmp0 = atomicrmw add i64* %p, i64 5 seq_cst<br>
> - %tmp1 = icmp ne i64 %tmp0, 0<br>
> - %tmp2 = zext i1 %tmp1 to i8<br>
> - ret i8 %tmp2<br>
> -}<br>
> -<br>
> attributes #0 = { nounwind }<br>
><br>
><br>
> ______________________________<wbr>_________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" class="cremed">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank" class="cremed">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>