Vector logic regression in r73431

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(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

Does the attached help?

-Eli

x.txt (1.03 KB)

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

No, that appears to be something unrelated. I'm currently using revision
75246, while that patch only seems to apply to some later revision.

I don't see the connection... anyway, I can't easily help you with an
old 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.

Right... my patch fixes that, I think.

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?

It applies to all elements of the vector.

-Eli

Oh...

Now I see. Your patch does address this issue. It seemed so unrelated at
first, and I didn't look closer at it after I was unable to apply it to my
(slightly) older revision. Sorry about that.

Anyway, I can confirm that your patch fixed the asserts and crashes I was
seeing. So as far as I'm concerned go ahead and commit it if you haven't
done so already.

Thanks!

Nicolas

Hi Eli,

Could you eternize that patch by committing it? :wink:

Thanks,

Nicolas