[PATCH] D30993: [InstCombine] Liberate assert in InstCombiner::visitZExt

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

In https://reviews.llvm.org/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.


https://reviews.llvm.org/D30993