[PATCH] D124997: [InstCombine] Fix scalable-vector bitwise select matching

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

In D124997#3494745 <https://reviews.llvm.org/D124997#3494745>, @spatel wrote:

> The code comments could be improved. There are many different potential patterns within this block, and scalable vectors just make it harder to keep it all straight. :)
> But after mucking around in here for a long time, I don't think the fix is sufficient. 
> I added a test with 7bad1d281c798929a <https://reviews.llvm.org/rG7bad1d281c798929ae1be44b8c8a1e0713151ea9> to try to uncover another potential bug.
> But I can't find a case currently where we would go wrong because code before here (ComputeNumSignBits) prevents matching a scalable vector with the right combination of bitcasts to trigger that bug.
>
> A better fix should do something like this:
>
>   // If this is a vector, we may need to cast to match the condition's length.
>   Type *SelTy = A->getType();
>   if (auto *VecTy = dyn_cast<VectorType>(Cond->getType())) {
>     // For a fixed or scalable vector get N from <{vscale x} N x iM>
>     unsigned Elts = VecTy->getElementCount().getKnownMinValue();
>     // For a fixed or scalable vector, get the value N x iM; for a scalar this is just M.
>     unsigned SelEltSize = SelTy->getPrimitiveSizeInBits().getKnownMinSize();
>     Type *EltTy = Builder.getIntNTy(SelEltSize / Elts);
>     SelTy = VectorType::get(EltTy, VecTy->getElementCount());
>   }
>
> I had a hard time making sense of the type and size APIs, so if anyone knows that better, please correct/improve if possible.

Thanks for digging in. I also found it very difficult to get scalable-vector tests which would //actually// trigger all possible cases we're trying to handle.

I think what you've got looks okay. I adjusted the comments a little.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124997/new/

https://reviews.llvm.org/D124997