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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen 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)
----------------
david-arm wrote:
> 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.
@ctetreau touched on that a bit on the mailing list yesterday, see http://lists.llvm.org/pipermail/llvm-dev/2020-April/141162.html.

AIUI:
* If the code only cares about fixed width vectors, use FixedVectorType and getNumElements.
* If the code only cares about scalable vectors, use ScalableVectorType and getMinNumElements  or getElementCount.
* If the code cares about both, use VectorType and getElementCount.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1570
 
       SDValue Op;
       if (EltVT.isFloatingPoint())
----------------
david-arm wrote:
> 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
You're right, I misread this code.

In that case, does it make sense to move the return from this function into the branches, so that you get:

``` if (constantVector) {
  ...
  return DAG.getBuildVector(..)
} else if (ConstantAggregateZero) {
  ...
  if (isa<ScalableVector>(..))
    return DAG.getSplatVector(..)
  else
    return DAG.getBuildVector(..);
}
llvm_unreachable("Unknown vector constant")```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78636