Hi All,
I found a regression which triggers the asserts: "Binary operator types
must
match!" and "Op types should be identical!". It's happening
with a piece of
vector code, and the asserts happen because a logic operation is attempted
between a vector and a scalar (which is not present in the original code,
but created by InstCombine).
It's caused by revision 73431. I was able to fix it by changing the
following (identical) lines in InstCombiner::visitAnd, visitOr and visitXor:
if (SimplifyDemandedInstructionBits(I))
return &I;
Into:
if (!isa<VectorType>(I.getType()) &&
SimplifyDemandedInstructionBits(I))
return &I;
Apparently SimplifyDemandedInstructionBits doesn't work correctly with
vector operands, sometimes replacing them with a scalar value?
So could anyone who knows the ins and outs of this code have a look at how
to make it handle vectors correctly? Or if that's not an option right now,
please revert the broken optimizations. Note that there might be more things
affected than visitAnd, visitOr and vistXor, I've only been able to identify
these so far with little knowledge of the actual code. I currently don't
have a reduced testcase, but if really necessary I can try to extract one.
Cheers,
Nicolas
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20090729/e2f23f6f/attachment.html>
On Wed, Jul 29, 2009 at 3:45 AM, Nicolas Capens<nicolas at capens.net> wrote:> So could anyone who knows the ins and outs of this code have a look at how > to make it handle vectors correctly? Or if that’s not an option right now, > please revert the broken optimizations. Note that there might be more things > affected than visitAnd, visitOr and vistXor, I’ve only been able to identify > these so far with little knowledge of the actual code. I currently don’t > have a reduced testcase, but if really necessary I can try to extract one.Does the attached help? -Eli -------------- next part -------------- Index: InstructionCombining.cpp ==================================================================--- InstructionCombining.cpp (revision 77486) +++ InstructionCombining.cpp (working copy) @@ -1014,7 +1014,7 @@ if ((DemandedMask & (RHSKnownZero|RHSKnownOne)) == DemandedMask) { // all known if ((RHSKnownOne & LHSKnownOne) == RHSKnownOne) { - Constant *AndC = ConstantInt::get(*Context, + Constant *AndC = ConstantInt::get(VTy, ~RHSKnownOne & DemandedMask); Instruction *And = BinaryOperator::CreateAnd(I->getOperand(0), AndC, "tmp"); @@ -1407,7 +1407,7 @@ // If the client is only demanding bits that we know, return the known // constant. if ((DemandedMask & (RHSKnownZero|RHSKnownOne)) == DemandedMask) { - Constant *C = ConstantInt::get(*Context, RHSKnownOne); + Constant *C = ConstantInt::get(VTy, RHSKnownOne); if (isa<PointerType>(V->getType())) C = Context->getConstantExprIntToPtr(C, V->getType()); return C;
Hi all,
Hi Eli,
No, that appears to be something unrelated. I'm currently using revision
75246, while that patch only seems to apply to some later revision.
Anyway, I actually located the real bug. Right at the end of
InstCombiner::SimplifyDemandedUseBits, there's this piece of code:
// If the client is only demanding bits that we know, return the known
// constant.
if ((DemandedMask & (RHSKnownZero|RHSKnownOne)) == DemandedMask) {
Constant *C = Context->getConstantInt(RHSKnownOne);
if (isa<PointerType>(V->getType()))
C = Context->getConstantExprIntToPtr(C, V->getType());
return C;
}
return false;
}
Note that C is a scalar integer, and so when V is actually a vector the type
isn't preserved.
I'm not entirely sure how this function is supposed to work with vectors
though. DemandedMask, KnownOne and KnownZero are APInt's (scalars) the size
of an element of the vector. So when, as the comment describes, only known
bits are demanded, does that apply to just one element of the vector or all?
Thanks,
Nicolas
-----Original Message-----
From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu] On
Behalf Of Eli Friedman
Sent: woensdag 29 juli 2009 20:44
To: LLVM Developers Mailing List
Subject: Re: [LLVMdev] Vector logic regression in r73431
On Wed, Jul 29, 2009 at 3:45 AM, Nicolas Capens<nicolas at capens.net>
wrote:> So could anyone who knows the ins and outs of this code have a look at
> how to make it handle vectors correctly? Or if that's not an option
> right now, please revert the broken optimizations. Note that there
> might be more things affected than visitAnd, visitOr and vistXor, I've
> only been able to identify these so far with little knowledge of the
> actual code. I currently don't have a reduced testcase, but if really
necessary I can try to extract one.
Does the attached help?
-Eli
Hi Eli, Could you eternize that patch by committing it? ;) Thanks, Nicolas -----Original Message----- From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu] On Behalf Of Eli Friedman Sent: woensdag 29 juli 2009 20:44 To: LLVM Developers Mailing List Subject: Re: [LLVMdev] Vector logic regression in r73431 On Wed, Jul 29, 2009 at 3:45 AM, Nicolas Capens<nicolas at capens.net> wrote:> So could anyone who knows the ins and outs of this code have a look at > how to make it handle vectors correctly? Or if that's not an option > right now, please revert the broken optimizations. Note that there > might be more things affected than visitAnd, visitOr and vistXor, I've > only been able to identify these so far with little knowledge of the > actual code. I currently don't have a reduced testcase, but if reallynecessary I can try to extract one. Does the attached help? -Eli