[PATCH] D82056: [SVE] Remove calls to VectorType::getNumElements from Transforms/Vectorize

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:339
+  auto *DestTy = dyn_cast<FixedVectorType>(I.getType());
+  auto *SrcTy = cast<FixedVectorType>(V->getType());
   if (!DestTy || I.getOperand(0)->getType() != SrcTy)
----------------
ctetreau wrote:
> spatel wrote:
> > ctetreau wrote:
> > > ctetreau wrote:
> > > > spatel wrote:
> > > > > IIUC, we can't safely cast to FixedVectorType at this point (the dyn_cast may have failed).
> > > > > 
> > > > > Should we add a test like this:
> > > > > 
> > > > > ```
> > > > > define <vscale x 4 x float> @scalable_bitcast_same_elt_size(<vscale x 4 x i32> %v) {
> > > > >   %shuf = shufflevector <vscale x 4 x i32> %v, <vscale x 4 x i32> undef, <vscale x 4 x i32> zeroinitializer
> > > > >   %r = bitcast <vscale x 4 x i32> %shuf to <vscale x 4 x float>
> > > > >   ret <vscale x 4 x float> %r
> > > > > }
> > > > > 
> > > > > ```
> > > > > 
> > > > > I would add that in a preliminary commit myself, but it already crashes somewhere in the TTI cost model.
> > > > I can take a look at adding this test case and seeing if I can get to this line with a scalable vector.
> > > Looking at this more closely, it occurs to me that the TTI stuff is completely unimplemented for scalable vectors currently, so I'm not going to be able to add this test case. That said, for now, the scope of the cast to FixedVectorType can be reduced to just the two calls to getNumElements(). In principle, it should be possible to implement this function in terms of ElementCount. I'll add a fixme for this.
> > > 
> > > I think this should be safe, assuming it passes all the tests.
> > I think there's still a potential bug in doing the plain `cast<>` even if we can't show it in a test at this time.
> > Ie, we should do:
> >   auto *SrcTy = dyn_cast<FixedVectorType>(V->getType());
> >   if (!DestTy || !SrcTy || ...)
> What is the bug? V must be nonnull because the matcher succeeded, and it must be a value with type VectorType, because it is the first argument to a shufflevector. What am I missing?
The diff has changed, so the above code is fine now. But the diff below is nakedly casting to `FixedVectorType`. How did we ensure that our generic `VectorType` values actually are `FixedVectorType`? Ie, if we re-arrange this code for some reason to move the TTI checks after the diff below, then my test example will crash on this line:
  unsigned DestNumElts = cast<FixedVectorType>(DestTy)->getNumElements();


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82056