[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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138682