[InstCombine] Liberate assert in InstCombiner::visitZExt

[bjope]

Summary

The call to canEvaluateZExtd in InstCombiner::visitZExt may
return with BitsToClear == SrcTy->getScalarSizeInBits(), but
there is an assert that BitsToClear should be smaller than
SrcTy->getScalarSizeInBits().

I have a test case that triggers the assert, but it only happens
for my downstream target. I've not been able to trigger it for
any upstream target.

The assert triggered for a piece of code such as this

%shr1 = lshr i16 undef, 15
...
%shr2 = lshr i16 %shr1, 1
%conv = zext i16 %shr2 to i32

Normally the lshr instructions are constant folded before we
visit the zext (that is why it is so hard to reproduce).
The original pattern, before instcombine, is of course a lot more
complicated in my test case. The shift count in the second lshr
is for example determined by the outcome of a PHI instruction.
It seems like other rewrites by instcombine leads up to
the pattern above. And then the zext is pulled from the
worklist, and visited (hitting the assert), before we detect
that the lshr instrucions can be constant folded.

Anyway, since the canEvaluateZExtd may return with BitsToClear
equal to SrcTy->getScalarSizeInBits(), and since the rewrite
that converts the expression type to avoid a zero extend works
also for the case where SrcBitsKept ends up being zero, then
it should be OK to liberate the assert to

assert(BitsToClear <= SrcTy->getScalarSizeInBits() &&
       "Unreasonable BitsToClear");

[hfinkel]

Should we just abort the transformation in this case? Otherwise we just create an and with 0?

lib/Transforms/InstCombine/InstCombineCasts.cpp

908 ↗

(On Diff #91910)

This assert message is not very useful. More useful might be:

"Can't clear more bits than in SrcTy"

[bjope]

In D30993#702147, @hfinkel wrote:

Should we just abort the transformation in this case? Otherwise we just create an and with 0?

As I see it there are at least four options:

1. Just handle this case as the general case. I belive that CreateAnd with 0 will be folded directly into a constant 0, so we never get the 'and'. The possible drawback is that EvaluateInDifferentType also rewrites the source expressions. The instructions added by EvaluateInDifferentType is later removed ("IC: DCE:")) since they are dead due to the and being folded away.
2. Add special handling of the situation when SrcBitsKept==0, to let make the constant fold more explicit in the code. Here we should avoid EvaluateInDifferentType, and explicitly replace the zext with a zero instead of using CreateAnd.
3. Bail out (return nullptr) if BitsToClear==SrcTy->getScalarSizeInBits().
4. Exit the if-statement when BitsToClear==SrcTy->getScalarSizeInBits() and continue looking at the other tranforms done by visitZExt.

I basically went for option 1 as it was least intrusive.
Since I haven't been able to write a simple test case that triggers the BitsToClear==SrcTy->getScalarSizeInBits() situation, and since it seems to happen very rarely (or otherwise someone else would have fixed this assert already), I do not want to go for option 2. If I could test this easily (adding a regression test for an in-tree-target), then I think option 2 would have been the obvious choice.

[bjope]

Updated the assert message (as suggested by hfinkel).
Minor updates to the summary.

[hfinkel]

Okay, this does seem like the safest fix. LGTM.

llvm/trunk/lib/Transforms/InstCombine/InstCombineCasts.cpp

Instruction *InstCombiner::visitZExt(ZExtInst &CI) {
 
   // Attempt to extend the entire input expression tree to the destination
   // type.   Only do this if the dest type is a simple type, don't convert the
   // expression tree to something weird like i93 unless the source is also
   // strange.
   unsigned BitsToClear;
   if ((DestTy->isVectorTy() || shouldChangeType(SrcTy, DestTy)) &&
       canEvaluateZExtd(Src, DestTy, BitsToClear, *this, &CI)) {
-    assert(BitsToClear < SrcTy->getScalarSizeInBits() &&
-           "Unreasonable BitsToClear");
+    assert(BitsToClear <= SrcTy->getScalarSizeInBits() &&
+           "Can't clear more bits than in SrcTy");
 
     // Okay, we can transform this!  Insert the new expression now.
     DEBUG(dbgs() << "ICE: EvaluateInDifferentType converting expression type"
           " to avoid zero extend: " << CI << '\n');
     Value *Res = EvaluateInDifferentType(Src, DestTy, false);
     assert(Res->getType() == DestTy);
 
     uint32_t SrcBitsKept = SrcTy->getScalarSizeInBits()-BitsToClear;