<div dir="ltr">Hi Oliver,<div><br></div><div>Thanks for catching this. It was a nasty bug. Fixed in r253195.</div><div><br></div><div>Cheers,</div><div><br></div><div>James</div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, 13 Nov 2015 at 13:36 Oliver Stannard via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi James,<br>
<br>
I've found a codegen fault with csmith that seems to be caused by this commit. Here's the reproducer for it:<br>
#include <stdio.h><br>
<br>
unsigned a = 1;<br>
struct {<br>
unsigned f0 : 1;<br>
unsigned f1 : 16;<br>
} d = {0, 0}, f = {0, 0};<br>
<br>
__attribute((noinline))<br>
void foo() {<br>
f.f0 = d.f0 < (a | 1);<br>
}<br>
<br>
int main() {<br>
foo();<br>
printf("checksum = %X\n", f.f0);<br>
}<br>
<br>
This should print "checksum = 1" because the comparison in foo will be true (d.f0 is 0, (a | 1) is 1). However, with this patch applied, it prints "checksum = 0".<br>
<br>
The foo function is being miscompiled, here's the IR for that function:<br>
@a = global i32 1, align 4<br>
@d = global { i8, i8, i8, i8 } { i8 0, i8 0, i8 0, i8 undef }, align 4<br>
@f = global { i8, i8, i8, i8 } { i8 0, i8 0, i8 0, i8 undef }, align 4<br>
define void @foo() noinline nounwind {<br>
entry:<br>
%bf.load = load i32, i32* bitcast ({ i8, i8, i8, i8 }* @d to i32*), align 4<br>
%bf.clear = and i32 %bf.load, 1<br>
%0 = load i32, i32* @a, align 4, !tbaa !3<br>
%or = or i32 %0, 1<br>
%cmp = icmp ne i32 %bf.clear, %or<br>
%conv = zext i1 %cmp to i32<br>
%bf.load1 = load i32, i32* bitcast ({ i8, i8, i8, i8 }* @f to i32*), align 4<br>
%bf.clear2 = and i32 %bf.load1, -2<br>
%bf.set = or i32 %conv, %bf.clear2<br>
store i32 %bf.set, i32* bitcast ({ i8, i8, i8, i8 }* @f to i32*), align 4<br>
ret void<br>
}<br>
<br>
Here's the assembly I get for this function (with this patch applied):<br>
foo:<br>
movw r0, :lower16:f<br>
movw r2, :lower16:d<br>
movt r0, :upper16:f<br>
movt r2, :upper16:d<br>
ldr r1, [r0]<br>
ldr r2, [r2]<br>
bfc r1, #0, #1<br>
bfi r1, r2, #0, #1<br>
str r1, [r0]<br>
bx lr<br>
<br>
Oliver<br>
<br>
> -----Original Message-----<br>
> From: llvm-commits [mailto:<a href="mailto:llvm-commits-bounces@lists.llvm.org" target="_blank">llvm-commits-bounces@lists.llvm.org</a>] On Behalf<br>
> Of James Molloy via llvm-commits<br>
> Sent: 10 November 2015 14:22<br>
> To: <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> Subject: [llvm] r252606 - Reapply "[ARM] Combine CMOV into BFI where<br>
> possible"<br>
><br>
> Author: jamesm<br>
> Date: Tue Nov 10 08:22:05 2015<br>
> New Revision: 252606<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=252606&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=252606&view=rev</a><br>
> Log:<br>
> Reapply "[ARM] Combine CMOV into BFI where possible"<br>
><br>
> Added fixes for stage2 failures: CMOV is not commutable; commuting the<br>
> operands results in the condition being flipped! d'oh!<br>
><br>
> Original commit message:<br>
><br>
> If we have a CMOV, OR and AND combination such as:<br>
> if (x & CN)<br>
> y |= CM;<br>
><br>
> And:<br>
> * CN is a single bit;<br>
> * All bits covered by CM are known zero in y;<br>
><br>
> Then we can convert this to a sequence of BFI instructions. This will<br>
> always be a win if CM is a single bit, will always be no worse than the<br>
> TST & OR sequence if CM is two bits, and for thumb will be no worse if CM<br>
> is three bits (due to the extra IT instruction).<br>
><br>
> Modified:<br>
> llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp<br>
> llvm/trunk/lib/Target/ARM/ARMISelLowering.h<br>
> llvm/trunk/test/CodeGen/ARM/bfi.ll<br>
><br>
> Modified: llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-</a><br>
> project/llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp?rev=252606&r1=252605<br>
> &r2=252606&view=diff<br>
> ==========================================================================<br>
> ====<br>
> --- llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp (original)<br>
> +++ llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp Tue Nov 10 08:22:05 2015<br>
> @@ -10241,6 +10241,111 @@ static SDValue PerformExtendCombine(SDNo<br>
> return SDValue();<br>
> }<br>
><br>
> +static void computeKnownBits(SelectionDAG &DAG, SDValue Op, APInt<br>
> &KnownZero,<br>
> + APInt &KnownOne) {<br>
> + if (Op.getOpcode() == ARMISD::BFI) {<br>
> + // Conservatively, we can recurse down the first operand<br>
> + // and just mask out all affected bits.<br>
> + computeKnownBits(DAG, Op.getOperand(0), KnownZero, KnownOne);<br>
> +<br>
> + // The operand to BFI is already a mask suitable for removing the<br>
> bits it<br>
> + // sets.<br>
> + ConstantSDNode *CI = cast<ConstantSDNode>(Op.getOperand(2));<br>
> + APInt Mask = CI->getAPIntValue();<br>
> + KnownZero &= Mask;<br>
> + KnownOne &= Mask;<br>
> + return;<br>
> + }<br>
> + if (Op.getOpcode() == ARMISD::CMOV) {<br>
> + APInt KZ2(KnownZero.getBitWidth(), 0);<br>
> + APInt KO2(KnownOne.getBitWidth(), 0);<br>
> + computeKnownBits(DAG, Op.getOperand(1), KnownZero, KnownOne);<br>
> + computeKnownBits(DAG, Op.getOperand(2), KZ2, KO2);<br>
> +<br>
> + KnownZero &= KZ2;<br>
> + KnownOne &= KO2;<br>
> + return;<br>
> + }<br>
> + return DAG.computeKnownBits(Op, KnownZero, KnownOne);<br>
> +}<br>
> +<br>
> +SDValue ARMTargetLowering::PerformCMOVToBFICombine(SDNode *CMOV,<br>
> SelectionDAG &DAG) const {<br>
> + // If we have a CMOV, OR and AND combination such as:<br>
> + // if (x & CN)<br>
> + // y |= CM;<br>
> + //<br>
> + // And:<br>
> + // * CN is a single bit;<br>
> + // * All bits covered by CM are known zero in y<br>
> + //<br>
> + // Then we can convert this into a sequence of BFI instructions. This<br>
> will<br>
> + // always be a win if CM is a single bit, will always be no worse than<br>
> the<br>
> + // TST&OR sequence if CM is two bits, and for thumb will be no worse if<br>
> CM is<br>
> + // three bits (due to the extra IT instruction).<br>
> +<br>
> + SDValue Op0 = CMOV->getOperand(0);<br>
> + SDValue Op1 = CMOV->getOperand(1);<br>
> + SDValue CmpZ = CMOV->getOperand(4);<br>
> +<br>
> + assert(CmpZ->getOpcode() == ARMISD::CMPZ);<br>
> + SDValue And = CmpZ->getOperand(0);<br>
> + if (And->getOpcode() != ISD::AND)<br>
> + return SDValue();<br>
> + ConstantSDNode *AndC = dyn_cast<ConstantSDNode>(And->getOperand(1));<br>
> + if (!AndC || !AndC->getAPIntValue().isPowerOf2())<br>
> + return SDValue();<br>
> + SDValue X = And->getOperand(0);<br>
> +<br>
> + if (Op1->getOpcode() != ISD::OR)<br>
> + return SDValue();<br>
> +<br>
> + ConstantSDNode *OrC = dyn_cast<ConstantSDNode>(Op1->getOperand(1));<br>
> + if (!OrC)<br>
> + return SDValue();<br>
> + SDValue Y = Op1->getOperand(0);<br>
> +<br>
> + if (Op0 != Y)<br>
> + return SDValue();<br>
> +<br>
> + // Now, is it profitable to continue?<br>
> + APInt OrCI = OrC->getAPIntValue();<br>
> + unsigned Heuristic = Subtarget->isThumb() ? 3 : 2;<br>
> + if (OrCI.countPopulation() > Heuristic)<br>
> + return SDValue();<br>
> +<br>
> + // Lastly, can we determine that the bits defined by OrCI<br>
> + // are zero in Y?<br>
> + APInt KnownZero, KnownOne;<br>
> + computeKnownBits(DAG, Y, KnownZero, KnownOne);<br>
> + if ((OrCI & KnownZero) != OrCI)<br>
> + return SDValue();<br>
> +<br>
> + // OK, we can do the combine.<br>
> + SDValue V = Y;<br>
> + SDLoc dl(X);<br>
> + EVT VT = X.getValueType();<br>
> + unsigned BitInX = AndC->getAPIntValue().logBase2();<br>
> +<br>
> + if (BitInX != 0) {<br>
> + // We must shift X first.<br>
> + X = DAG.getNode(ISD::SRL, dl, VT, X,<br>
> + DAG.getConstant(BitInX, dl, VT));<br>
> + }<br>
> +<br>
> + for (unsigned BitInY = 0, NumActiveBits = OrCI.getActiveBits();<br>
> + BitInY < NumActiveBits; ++BitInY) {<br>
> + if (OrCI[BitInY] == 0)<br>
> + continue;<br>
> + APInt Mask(VT.getSizeInBits(), 0);<br>
> + Mask.setBit(BitInY);<br>
> + V = DAG.getNode(ARMISD::BFI, dl, VT, V, X,<br>
> + // Confusingly, the operand is an *inverted* mask.<br>
> + DAG.getConstant(~Mask, dl, VT));<br>
> + }<br>
> +<br>
> + return V;<br>
> +}<br>
> +<br>
> /// PerformCMOVCombine - Target-specific DAG combining for ARMISD::CMOV.<br>
> SDValue<br>
> ARMTargetLowering::PerformCMOVCombine(SDNode *N, SelectionDAG &DAG) const<br>
> {<br>
> @@ -10259,6 +10364,13 @@ ARMTargetLowering::PerformCMOVCombine(SD<br>
> ARMCC::CondCodes CC =<br>
> (ARMCC::CondCodes)cast<ConstantSDNode>(ARMcc)->getZExtValue();<br>
><br>
> + // BFI is only available on V6T2+.<br>
> + if (!Subtarget->isThumb1Only() && Subtarget->hasV6T2Ops()) {<br>
> + SDValue R = PerformCMOVToBFICombine(N, DAG);<br>
> + if (R)<br>
> + return R;<br>
> + }<br>
> +<br>
> // Simplify<br>
> // mov r1, r0<br>
> // cmp r1, x<br>
><br>
> Modified: llvm/trunk/lib/Target/ARM/ARMISelLowering.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-</a><br>
> project/llvm/trunk/lib/Target/ARM/ARMISelLowering.h?rev=252606&r1=252605&r<br>
> 2=252606&view=diff<br>
> ==========================================================================<br>
> ====<br>
> --- llvm/trunk/lib/Target/ARM/ARMISelLowering.h (original)<br>
> +++ llvm/trunk/lib/Target/ARM/ARMISelLowering.h Tue Nov 10 08:22:05 2015<br>
> @@ -260,6 +260,7 @@ namespace llvm {<br>
> SDNode *Node) const override;<br>
><br>
> SDValue PerformCMOVCombine(SDNode *N, SelectionDAG &DAG) const;<br>
> + SDValue PerformCMOVToBFICombine(SDNode *N, SelectionDAG &DAG) const;<br>
> SDValue PerformDAGCombine(SDNode *N, DAGCombinerInfo &DCI) const<br>
> override;<br>
><br>
> bool isDesirableToTransformToIntegerOp(unsigned Opc, EVT VT) const<br>
> override;<br>
><br>
> Modified: llvm/trunk/test/CodeGen/ARM/bfi.ll<br>
> URL: <a href="http://llvm.org/viewvc/llvm-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-</a><br>
> project/llvm/trunk/test/CodeGen/ARM/bfi.ll?rev=252606&r1=252605&r2=252606&<br>
> view=diff<br>
> ==========================================================================<br>
> ====<br>
> --- llvm/trunk/test/CodeGen/ARM/bfi.ll (original)<br>
> +++ llvm/trunk/test/CodeGen/ARM/bfi.ll Tue Nov 10 08:22:05 2015<br>
> @@ -74,3 +74,37 @@ entry:<br>
> %or = or i32 %shl, %and<br>
> ret i32 %or<br>
> }<br>
> +<br>
> +define i32 @f7(i32 %x, i32 %y) {<br>
> +; CHECK-LABEL: f7:<br>
> +; CHECK: bfi r1, r0, #4, #1<br>
> + %y2 = and i32 %y, 4294967040 ; 0xFFFFFF00<br>
> + %and = and i32 %x, 4<br>
> + %or = or i32 %y2, 16<br>
> + %cmp = icmp ne i32 %and, 0<br>
> + %sel = select i1 %cmp, i32 %or, i32 %y2<br>
> + ret i32 %sel<br>
> +}<br>
> +<br>
> +define i32 @f8(i32 %x, i32 %y) {<br>
> +; CHECK-LABEL: f8:<br>
> +; CHECK: bfi r1, r0, #4, #1<br>
> +; CHECK: bfi r1, r0, #5, #1<br>
> + %y2 = and i32 %y, 4294967040 ; 0xFFFFFF00<br>
> + %and = and i32 %x, 4<br>
> + %or = or i32 %y2, 48<br>
> + %cmp = icmp ne i32 %and, 0<br>
> + %sel = select i1 %cmp, i32 %or, i32 %y2<br>
> + ret i32 %sel<br>
> +}<br>
> +<br>
> +define i32 @f9(i32 %x, i32 %y) {<br>
> +; CHECK-LABEL: f9:<br>
> +; CHECK-NOT: bfi<br>
> + %y2 = and i32 %y, 4294967040 ; 0xFFFFFF00<br>
> + %and = and i32 %x, 4<br>
> + %or = or i32 %y2, 48<br>
> + %cmp = icmp ne i32 %and, 0<br>
> + %sel = select i1 %cmp, i32 %y2, i32 %or<br>
> + ret i32 %sel<br>
> +}<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br>
________________________________<br>
<br>
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>