[PATCH] D78636: [CodeGen] Use SPLAT_VECTOR for zeroinitialiser with scalable types

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1562
     if (const ConstantVector *CV = dyn_cast<ConstantVector>(C)) {
-      for (unsigned i = 0; i != NumElements; ++i)
+      assert(!EltCnt.Scalable && "ConstantVector used for scalable types");
+      for (unsigned i = 0; i != EltCnt.Min; ++i)
----------------
sdesmalen wrote:
> D77587 introduced FixedVectorType and ScalableVectorType, which means you can instead write:
>   cast<FixedVectorType>(VecTy)->getNumElements()
> That also removes the need for an additional assert.
OK sure. It's just that I already got the ElementCount above from VecTy so I wasn't sure it was worth casting VecTy to a FixedVectorType here - it seemd a bit redundant. What's the preferred style in general here? Avoiding using ElementCount wherever possible and cast to Fixed and Scalable types? It would be good to have a consistent style because there are two ways of reaching the same answer.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1570
 
       SDValue Op;
       if (EltVT.isFloatingPoint())
----------------
sdesmalen wrote:
> I'd prefer to see the Scalable and FixedWidth cases more separated here, something like
> 
>   if (isa<ScalableVectorType>(...))
>     // Op = splat vector
>   else {
>     if (EltVT.isFloatingPoint())
>       ...
>     ...
>   }
I'm not sure what you mean here. Are you suggesting duplicating the code into two separate cases? We still need to check whether it is floating point in either case, Wouldn't this be better?

      assert(isa<ConstantAggregateZero>(C) && "Unknown vector constant!");
      EVT EltVT =
          TLI.getValueType(DAG.getDataLayout(), VecTy->getElementType());

      SDValue Op;
      if (EltVT.isFloatingPoint())
        Op = DAG.getConstantFP(0, getCurSDLoc(), EltVT);
      else
        Op = DAG.getConstant(0, getCurSDLoc(), EltVT);

      if (isa<ScalableVectorType>(VecTy))
        return NodeMap[V] = DAG.getSplatVector(VT, getCurSDLoc(), Op);

      ...  Continue as normal for FixedVectorType


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78636