[CodeGen] Fix FoldConstantVectorArithmetic for scalable vectors

[david-arm]

Summary

For now I have changed FoldConstantVectorArithmetic to return early
if we encounter a scalable vector, since the subsequent code assumes
you can perform lane-wise constant folds. However, in future work we
should be able to extend this to look at splats of a constant value
and fold those if possible. I have also added the same code to
FoldConstantArithmetic, since that deals with vectors too.

The warnings I fixed in this patch were being generated by this
existing test:

CodeGen/AArch64/sve-int-arith.ll

[RKSimon]

You might want to add something similar to FoldConstantArithmetic as FoldConstantVectorArithmetic will be soon folded into it.

[fpetrogalli]

The code path I have changed is defended by this existing test:

CodeGen/AArch64/sve-int-arith.ll

Hi @david-arm ,

NIT1: in what way does the test defend the change? The test is successful with and without your changes, so I think this comment in the commit message is misleading I know you have added this to remove the warnings generated when using getVectorNumElements on scalable vectors, so my personal preference is that there is nothing to defend here, it is just a sensible change to do to avoid entering code that makes invalid assumption on the underlying VTs. I think you can remove the paragraph I quoted from the commit message.

Grazie!

Francesco

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

4918

NIT2: In the spirit of NIT1, I think you should add the TODO only on the part that requires implementing constant folding for splat into scalable vectors, as follows:

// Custom code for scalable vectors, as all the folds below assume a fixed width vector.
// TODO: bail off for now, however we should be able to do constant folds involving splats to scalable vectors too.

This way, once the TODO is done and removed, we will not loose the information on why the scalable vectors require custom code.

5001–5003

Ditto.

[david-arm]

Re NIT1: all I mean is that these tests exercise these code paths. Previously we were fortunate enough that it just "worked", but now I've added to *ensure* that it is guaranteed to work. So in that sense the tests are testing that we don't do something silly in the places I've changed.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

4918

Hi Francesco, I'm sorry but I don't quite follow here. All the code below this is not custom code for scalable vectors. Even if we later fixed the code below to work for both fixed and scalable, it still won't be specific to scalable.

[fpetrogalli]

LGTM, please consider adding those two comments I mentioned in my last review before submitting.

Francesco

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

4918

I wanted to make sure that there was a clear cut between the code for scalable vectors and the code for fixed width vector. So maybe it makes sense to leave the comment as it is, and just modify line 4952 to say:

// For fixed width vectors, extract each constant element and fold them individually.

5008

Same here, add a comment saying that from now on the vectors are assumed to be fixed width.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

SDValue SelectionDAG::FoldConstantArithmetic(unsigned Opcode, const SDLoc &DL,
 
   // fold (add Sym, c) -> Sym+c
   if (GlobalAddressSDNode *GA = dyn_cast<GlobalAddressSDNode>(N1))
     return FoldSymbolOffset(Opcode, VT, GA, N2);
   if (TLI->isCommutativeBinOp(Opcode))
     if (GlobalAddressSDNode *GA = dyn_cast<GlobalAddressSDNode>(N2))
       return FoldSymbolOffset(Opcode, VT, GA, N1);
 
-  // For vectors, extract each constant element and fold them individually.
-  // Either input may be an undef value.
+  // TODO: All the folds below are performed lane-by-lane and assume a fixed
+  // vector width, however we should be able to do constant folds involving
+  // splat vector nodes too.
+  if (VT.isScalableVector())
+    return SDValue();
+
+  // For fixed width vectors, extract each constant element and fold them
+  // individually. Either input may be an undef value.
   auto *BV1 = dyn_cast<BuildVectorSDNode>(N1);
   if (!BV1 && !N1->isUndef())
     return SDValue();
   auto *BV2 = dyn_cast<BuildVectorSDNode>(N2);
   if (!BV2 && !N2->isUndef())
     return SDValue();
   // If both operands are undef, that's handled the same way as scalars.
   if (!BV1 && !BV2)
SDValue SelectionDAG::FoldConstantVectorArithmetic(unsigned Opcode,
 
   if (isUndef(Opcode, Ops))
     return getUNDEF(VT);
 
   // We can only fold vectors - maybe merge with FoldConstantArithmetic someday?
   if (!VT.isVector())
     return SDValue();
 
+  // TODO: All the folds below are performed lane-by-lane and assume a fixed
+  // vector width, however we should be able to do constant folds involving
+  // splat vector nodes too.
+  if (VT.isScalableVector())
+    return SDValue();
+
+  // From this point onwards all vectors are assumed to be fixed width.
   unsigned NumElts = VT.getVectorNumElements();
 
   auto IsScalarOrSameVectorSize = [&](const SDValue &Op) {
     return !Op.getValueType().isVector() ||
            Op.getValueType().getVectorNumElements() == NumElts;
   };
 
   auto IsConstantBuildVectorOrUndef = [&](const SDValue &Op) {
     BuildVectorSDNode *BV = dyn_cast<BuildVectorSDNode>(Op);