[PATCH] D138682: [AArch64][SME]: Generate streaming-compatible code for bit counting/select

[mgabka via Phabricator via llvm-commits]

mgabka added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15694
   // it does not work for SVE when dealing with vectors wider than 128 bits.
-  if (!VT.is64BitVector() && !VT.is128BitVector())
+  if (!VT.is64BitVector() && !VT.is128BitVector() ||
+      DAG.getSubtarget<AArch64Subtarget>().forceStreamingCompatibleSVE())
----------------
sdesmalen wrote:
> mgabka wrote:
> > sdesmalen wrote:
> > > mgabka wrote:
> > > > david-arm wrote:
> > > > > I think you need brackets around the first part, i.e.
> > > > > 
> > > > >   if ((!VT.is64BitVector() && !VT.is128BitVector()) ||
> > > > I think this code is introducing a bug, before the combine had an early exit if the VT wasn't 64 or 128 bit vector, while now we allow scalable vectors, and the combine does not give correct results for scalable vectors, it is just not triggered because the "ISD::isBuildVectorAllZeros" and "ISD::isBuildVectorAllOnes" reject vector splat, but I think it would be better to have a clear early exit for scalable VT.
> > > > What do you think? 
> > > From what I can see, `useSVEForFixedLengthVectorVT` returns `false` if `VT` is a scalable vector, so I don't think this would be a problem.
> > exactly it returns false, so the branch is not taken and there is no early exit, while before we would have an early exit.
> Okay I see what you mean now. Yes, that does seem like a functional change. Did you find this by looking at the code, or did you come across a regression somewhere?
I had a downstream regression.

Looks like in upstream LLVM there is no tests for it, but the code below relies on VT.getScalarSizeInBits() being 64 or 128, check line 15720, so although for scalable vector luckily we are safe (thanks to the fact that ISD::isBuildVectorAllZeros does not accept spats), I think we might have issues for fixed width vectors where SVE should not be used, and where  VT.getScalarSizeInBits() isn't 64 nor 128.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138682