This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Use SPLAT_VECTOR for zeroinitialiser with scalable types
ClosedPublic

Authored by david-arm on Apr 22 2020, 6:49 AM.

Details

Summary

When generating code for the LLVM IR zeroinitialiser operation, if
the vector type is scalable we should be using SPLAT_VECTOR instead
of BUILD_VECTOR.

Diff Detail

Event Timeline

david-arm created this revision.Apr 22 2020, 6:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2020, 6:49 AM
sdesmalen added inline comments.Apr 23 2020, 6:29 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1562

D77587 introduced FixedVectorType and ScalableVectorType, which means you can instead write:

cast<FixedVectorType>(VecTy)->getNumElements()

That also removes the need for an additional assert.

1570

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())
    ...
  ...
}
1576

You can then write:

if (isa<ScalableVectorType>(VecTy))
  Op = DAG.getSplatVector(VT, getCurSDLoc(), Op);
david-arm added inline comments.Apr 23 2020, 7:26 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1562

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.

1570

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
sdesmalen added inline comments.Apr 23 2020, 7:58 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1562

@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.
1570

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")
david-arm updated this revision to Diff 259835.Apr 24 2020, 2:21 AM
david-arm marked 3 inline comments as done.
david-arm updated this revision to Diff 259853.Apr 24 2020, 3:48 AM
sdesmalen accepted this revision.Apr 24 2020, 4:44 AM

LGTM

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1587

nit: unnecessary newline.

1591

nit: the else is not necessary here.

This revision is now accepted and ready to land.Apr 24 2020, 4:44 AM
This revision was automatically updated to reflect the committed changes.