[CodeGen] Fix warnings due to SelectionDAG::getSplatSourceVector

[david-arm]

Summary

I have fixed several places in getSplatSourceVector and isSplatValue
to work correctly with scalable vectors. I added new support for
the ISD::SPLAT_VECTOR DAG node as one of the obvious cases we can
support with scalable vectors. In other places I have tried to do
the sensible thing, such as bail out for vector types we don't yet
support or don't intend to support.

It's not possible to add IR test cases to cover these changes, since
they are currently only ever exercised on certain targets, e.g.
only X86 targets use the result of getSplatSourceVector. I've
assumed that X86 tests already exist to test these code paths for
fixed vectors. However, I have added some AArch64 unit tests that
test the specific functions I have changed.

[fpetrogalli]

Hi @david-arm ,

I think you could tests some of these methods of SelectionDAG directly like it has been done in llvm/unittest/CodeGen/AArch64SelectionDAGTest.cpp.

You can reuse the infrastructure in there to create the DAG instance, and from there build the values you want to test for the methods you have modified.

Francesco

[david-arm]

OK @fpetrogalli I'll take a look!

[fpetrogalli]

Hi @david-arm ,

thanks for adding the unit tests!

I have added few comments, most are repetitive...

You'll notice I am asking what DemandedElts and UndefElts references are for, and to add a comment in the code to explain. I think that while reviewing the code I understood what they are for (yes, I started from the tests, not from the implementation), but I still think that it would be good to expand the doxygen docs with their description.

Have a good weekend!

Francesco

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

2279–2281

While we are looking at this function, let's not miss the opportunity to expand the docs here and explain what demanded elts and undef elts are! :)

2287–2288

Given that DemandedElts seems to make sense only for fixed size vectors, I think you should move this after the if (VT.isScalableVector()) return false; on line 2310, to make sure that all the code related to fixed size is run all together after the generic code of the switch.

Also, given that DemendedElts seem to make no sense for scalable vectors, I wonder if it is worth adding an assertion here along the lines of

assert((!VF.isScalable() || !DemandedElts) && "No access to arbitrary lanes at compile time for scalable vectors");

2391

This seems also a good candidate to unit test, at least for the three cases you have modified?

llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp

202

The tests would benefit of a couple of lines of comment explaining what you are testing...

210

Please add a comment to all constant parameters with the name of the parameters. Here and in the rest of the tests you have added.

211

Avoid auto when the type is not explicit in the RHS.

213

EXPECT_TRUE(DAG->isSplatValue(Op, false))

217

Similarly, use EXPECT_FALSE

220

EXPECT_TRUE

Question: what does DemandedElts = APInt(16, 3); changes from the invocation of isSplatValue at line 217 to modify its answer? A comment in the code would be much appreciated! :)
Background: I have no idea what DemandedElts mean, and why (16, 3) would change the previous answer. What would (33, 1) do?

234–235

Please don't use auto here.

239

EXPECT_TRUE

243

EXPECT_FALSE

246

EXPECT_TRUE

258

Don't use auto here.

260

EXPECT_TRUE

264

EXPECT_TRUE. I am too lazy to mark the remaining, and to do the reasonable thing to remove the previous one and add a generic comment to the review. :) Please fix all uses of expect with booleans.

266

Yeah, so, why do you always use APInt(16, 3)? Can you vary those values?

282–283

Don't use auto.

285

Don't use auto.

[david-arm]

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

2279–2281

Hi Francesco, DemandedElts and UndefElts are used widely throughout the code base for many similarly named functions in both CodeGen and IR, so I don't think it makes sense to go into huge detail for these few functions alone. However, I can certainly update the comments for the functions I've changed and explain how DemandedElts and UndefElts are meaningless for scalable vectors.

2287–2288

I deliberately wanted the "if (!VT.isScalableVector() && !DemandedElts)" check at the start because there is no point doing any further work for fixed width vectors if the caller does not care about any of the elements.

2287–2288

With regards to adding an assert, you raise a good point. However, adding that assert will break my unit tests too so I would have to fix those up. Also, there is precedence in this area - see https://reviews.llvm.org/D79053, where asserts were not added for scalable vectors in similar cases. I'm not sure on this one - I'll have a think about it.

2391

Sure, I'd forgotten to test this. Sigh, writing unit tests is way more time consuming than the code changes themselves. :)

llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp

202

Fair point! I'll add comments.

210

Sure I can add a comment, but I'm not sure what value it would add because the value of the constant is irrevelant. Just a convenient way to get a splat that's all. I can say exactly that if you think that's useful?

211

See my respone to the last 'auto' comment.

234–235

See my respone to the last 'auto' comment.

258

See my respone to the last 'auto' comment.

264

OK sure, seems reasonable.

266

I didn't really see the need to vary the values to be honest. I waasn't sure what value it would add to the tests.

282–283

See my respone to the last 'auto' comment.

285

Hi Francesco, I use 'auto' here because the rest of the tests in this file does. I personally believe it is more important to follow the convention of the existing file, so if possible I'd like to leave these in.

[david-arm]

llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp

285

Hi again, so I'm not sure what the LLVM coding standards are in this case - whether convention in an existing file takes priority or avoiding the use of "auto" takes priority. Sander also mentioned that it's probably more important to avoid 'auto' in new functions even in a file where it is used everywhere.

[fpetrogalli]

Hi @david-arm ,

thank you for addressing (almost) all comments. Please notice that my reading of the coding standards would require to name at least the boolean parameters in a call. OK, maybe the getters of llvm::Vector are widely used an so adding comments to name them is not very important, but I think it would be important for things like DAG->isSplatValue(Op, false). I accepted the patch but please consider adding such comments before submitting.

Last request: for future reference, may I ask you to mark the comments you have addressed as done? It is quite easy to loose track of them otherwise.

Cheers!

Francesco

llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp

210

For the record, I didn't come up with this idea myself: https://llvm.org/docs/CodingStandards.html#comment-formatting

Admittedly, I might have pushed it a bit when I say "all parameters", but the text in the link seems to hint that bool constant should be named with a comment.

To me, the following change provides essential information that is worth having.

auto VecVT = EVT::getVectorVT(Context, IntVT, /*NumElements=/16, /*IsScalable=*/false);

285

Yep, couldn't agree more with Sander :)

https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

 
 /// MaskedValueIsAllOnes - Return true if '(Op & Mask) == Mask'.
 bool SelectionDAG::MaskedValueIsAllOnes(SDValue V, const APInt &Mask,
                                         unsigned Depth) const {
   return Mask.isSubsetOf(computeKnownBits(V, Depth).One);
 }
 
 /// isSplatValue - Return true if the vector V has the same value
-/// across all DemandedElts.
+/// across all DemandedElts. For scalable vectors it does not make
+/// sense to specify which elements are demanded or undefined, therefore
+/// they are simply ignored.
 bool SelectionDAG::isSplatValue(SDValue V, const APInt &DemandedElts,
                                 APInt &UndefElts) {
-  if (!DemandedElts)
-    return false; // No demanded elts, better to assume we don't know anything.
-
   EVT VT = V.getValueType();
   assert(VT.isVector() && "Vector type expected");
 
+  if (!VT.isScalableVector() && !DemandedElts)
+    return false; // No demanded elts, better to assume we don't know anything.
+
+  // Deal with some common cases here that work for both fixed and scalable
+  // vector types.
+  switch (V.getOpcode()) {
+  case ISD::SPLAT_VECTOR:
+    return true;
+  case ISD::ADD:
+  case ISD::SUB:
+  case ISD::AND: {
+    APInt UndefLHS, UndefRHS;
+    SDValue LHS = V.getOperand(0);
+    SDValue RHS = V.getOperand(1);
+    if (isSplatValue(LHS, DemandedElts, UndefLHS) &&
+        isSplatValue(RHS, DemandedElts, UndefRHS)) {
+      UndefElts = UndefLHS | UndefRHS;
+      return true;
+    }
+    break;
+  }
+  }
+
+  // We don't support other cases than those above for scalable vectors at
+  // the moment.
+  if (VT.isScalableVector())
+    return false;
+
   unsigned NumElts = VT.getVectorNumElements();
   assert(NumElts == DemandedElts.getBitWidth() && "Vector size mismatch");
   UndefElts = APInt::getNullValue(NumElts);
 
   switch (V.getOpcode()) {
   case ISD::BUILD_VECTOR: {
     SDValue Scl;
     for (unsigned i = 0; i != NumElts; ++i) {
if (SubIdx && SubIdx->getAPIntValue().ule(NumSrcElts - NumElts)) {
       APInt DemandedSrc = DemandedElts.zextOrSelf(NumSrcElts).shl(Idx);
       if (isSplatValue(Src, DemandedSrc, UndefSrcElts)) {
         UndefElts = UndefSrcElts.extractBits(NumElts, Idx);
         return true;
       }
     }
     break;
   }
-  case ISD::ADD:
-  case ISD::SUB:
-  case ISD::AND: {
-    APInt UndefLHS, UndefRHS;
-    SDValue LHS = V.getOperand(0);
-    SDValue RHS = V.getOperand(1);
-    if (isSplatValue(LHS, DemandedElts, UndefLHS) &&
-        isSplatValue(RHS, DemandedElts, UndefRHS)) {
-      UndefElts = UndefLHS | UndefRHS;
-      return true;
-    }
-    break;
-  }
   }
 
   return false;
 }
 
 /// Helper wrapper to main isSplatValue function.
 bool SelectionDAG::isSplatValue(SDValue V, bool AllowUndefs) {
   EVT VT = V.getValueType();
   assert(VT.isVector() && "Vector type expected");
-  unsigned NumElts = VT.getVectorNumElements();
 
   APInt UndefElts;
-  APInt DemandedElts = APInt::getAllOnesValue(NumElts);
+  APInt DemandedElts;
+
+  // For now we don't support this with scalable vectors.
+  if (!VT.isScalableVector())
+    DemandedElts = APInt::getAllOnesValue(VT.getVectorNumElements());
   return isSplatValue(V, DemandedElts, UndefElts) &&
          (AllowUndefs || !UndefElts);
 }
 
 SDValue SelectionDAG::getSplatSourceVector(SDValue V, int &SplatIdx) {
   V = peekThroughExtractSubvectors(V);
 
   EVT VT = V.getValueType();
   unsigned Opcode = V.getOpcode();
   switch (Opcode) {
   default: {
     APInt UndefElts;
-    APInt DemandedElts = APInt::getAllOnesValue(VT.getVectorNumElements());
+    APInt DemandedElts;
+
+    if (!VT.isScalableVector())
+      DemandedElts = APInt::getAllOnesValue(VT.getVectorNumElements());
+
     if (isSplatValue(V, DemandedElts, UndefElts)) {
+      if (VT.isScalableVector()) {
+        // DemandedElts and UndefElts are ignored for scalable vectors, since
+        // the only supported cases are SPLAT_VECTOR nodes.
+        SplatIdx = 0;
+      } else {
         // Handle case where all demanded elements are UNDEF.
         if (DemandedElts.isSubsetOf(UndefElts)) {
           SplatIdx = 0;
           return getUNDEF(VT);
         }
         SplatIdx = (UndefElts & DemandedElts).countTrailingOnes();
+      }
       return V;
     }
     break;
   }
+  case ISD::SPLAT_VECTOR:
+    SplatIdx = 0;
+    return V;
   case ISD::VECTOR_SHUFFLE: {
+    if (VT.isScalableVector())
+      return SDValue();
+
     // Check if this is a shuffle node doing a splat.
     // TODO - remove this and rely purely on SelectionDAG::isSplatValue,
     // getTargetVShiftNode currently struggles without the splat source.
     auto *SVN = cast<ShuffleVectorSDNode>(V);
     if (!SVN->isSplat())
       break;
     int Idx = SVN->getSplatIndex();
     int NumElts = V.getValueType().getVectorNumElements();

llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp

TEST_F(AArch64SelectionDAGTest, ComputeKnownBits_SUB) {
   //  =>
   // Known.One  = 00000001 (0x1)
   // Known.Zero = 10000000 (0x80)
   KnownBits Known = DAG->computeKnownBits(Op);
   EXPECT_EQ(Known.Zero, APInt(8, 0x80));
   EXPECT_EQ(Known.One, APInt(8, 0x1));
 }
 
+TEST_F(AArch64SelectionDAGTest, isSplatValue_Fixed_BUILD_VECTOR) {
+  if (!TM)
+    return;
+
+  TargetLowering TL(*TM);
+
+  SDLoc Loc;
+  auto IntVT = EVT::getIntegerVT(Context, 8);
+  auto VecVT = EVT::getVectorVT(Context, IntVT, 16, false);
+  // Create a BUILD_VECTOR
+  SDValue Op = DAG->getConstant(1, Loc, VecVT);
+  EXPECT_EQ(Op->getOpcode(), ISD::BUILD_VECTOR);
+  EXPECT_TRUE(DAG->isSplatValue(Op, /*AllowUndefs=*/false));
+
+  APInt UndefElts;
+  APInt DemandedElts;
+  EXPECT_FALSE(DAG->isSplatValue(Op, DemandedElts, UndefElts));
+
+  // Width=16, Mask=3
+  DemandedElts = APInt(16, 3);
+  EXPECT_TRUE(DAG->isSplatValue(Op, DemandedElts, UndefElts));
+}
+
+TEST_F(AArch64SelectionDAGTest, isSplatValue_Fixed_ADD_of_BUILD_VECTOR) {
+  if (!TM)
+    return;
+
+  TargetLowering TL(*TM);
+
+  SDLoc Loc;
+  auto IntVT = EVT::getIntegerVT(Context, 8);
+  auto VecVT = EVT::getVectorVT(Context, IntVT, 16, false);
+
+  // Should create BUILD_VECTORs
+  SDValue Val1 = DAG->getConstant(1, Loc, VecVT);
+  SDValue Val2 = DAG->getConstant(3, Loc, VecVT);
+  EXPECT_EQ(Val1->getOpcode(), ISD::BUILD_VECTOR);
+  SDValue Op = DAG->getNode(ISD::ADD, Loc, VecVT, Val1, Val2);
+
+  EXPECT_TRUE(DAG->isSplatValue(Op, /*AllowUndefs=*/false));
+
+  APInt UndefElts;
+  APInt DemandedElts;
+  EXPECT_FALSE(DAG->isSplatValue(Op, DemandedElts, UndefElts));
+
+  // Width=16, Mask=3
+  DemandedElts = APInt(16, 3);
+  EXPECT_TRUE(DAG->isSplatValue(Op, DemandedElts, UndefElts));
+}
+
+TEST_F(AArch64SelectionDAGTest, isSplatValue_Scalable_SPLAT_VECTOR) {
+  if (!TM)
+    return;
+
+  TargetLowering TL(*TM);
+
+  SDLoc Loc;
+  auto IntVT = EVT::getIntegerVT(Context, 8);
+  auto VecVT = EVT::getVectorVT(Context, IntVT, 16, true);
+  // Create a SPLAT_VECTOR
+  SDValue Op = DAG->getConstant(1, Loc, VecVT);
+  EXPECT_EQ(Op->getOpcode(), ISD::SPLAT_VECTOR);
+  EXPECT_TRUE(DAG->isSplatValue(Op, /*AllowUndefs=*/false));
+
+  APInt UndefElts;
+  APInt DemandedElts;
+  EXPECT_TRUE(DAG->isSplatValue(Op, DemandedElts, UndefElts));
+
+  // Width=16, Mask=3. These bits should be ignored.
+  DemandedElts = APInt(16, 3);
+  EXPECT_TRUE(DAG->isSplatValue(Op, DemandedElts, UndefElts));
+}
+
+TEST_F(AArch64SelectionDAGTest, isSplatValue_Scalable_ADD_of_SPLAT_VECTOR) {
+  if (!TM)
+    return;
+
+  TargetLowering TL(*TM);
+
+  SDLoc Loc;
+  auto IntVT = EVT::getIntegerVT(Context, 8);
+  auto VecVT = EVT::getVectorVT(Context, IntVT, 16, true);
+
+  // Should create SPLAT_VECTORS
+  SDValue Val1 = DAG->getConstant(1, Loc, VecVT);
+  SDValue Val2 = DAG->getConstant(3, Loc, VecVT);
+  EXPECT_EQ(Val1->getOpcode(), ISD::SPLAT_VECTOR);
+  SDValue Op = DAG->getNode(ISD::ADD, Loc, VecVT, Val1, Val2);
+
+  EXPECT_TRUE(DAG->isSplatValue(Op, /*AllowUndefs=*/false));
+
+  APInt UndefElts;
+  APInt DemandedElts;
+  EXPECT_TRUE(DAG->isSplatValue(Op, DemandedElts, UndefElts));
+
+  // Width=16, Mask=3. These bits should be ignored.
+  DemandedElts = APInt(16, 3);
+  EXPECT_TRUE(DAG->isSplatValue(Op, DemandedElts, UndefElts));
+}
+
+TEST_F(AArch64SelectionDAGTest, getSplatSourceVector_Fixed_BUILD_VECTOR) {
+  if (!TM)
+    return;
+
+  TargetLowering TL(*TM);
+
+  SDLoc Loc;
+  auto IntVT = EVT::getIntegerVT(Context, 8);
+  auto VecVT = EVT::getVectorVT(Context, IntVT, 16, false);
+  // Create a BUILD_VECTOR
+  SDValue Op = DAG->getConstant(1, Loc, VecVT);
+  EXPECT_EQ(Op->getOpcode(), ISD::BUILD_VECTOR);
+
+  int SplatIdx = -1;
+  EXPECT_EQ(DAG->getSplatSourceVector(Op, SplatIdx), Op);
+  EXPECT_EQ(SplatIdx, 0);
+}
+
+TEST_F(AArch64SelectionDAGTest, getSplatSourceVector_Fixed_ADD_of_BUILD_VECTOR) {
+  if (!TM)
+    return;
+
+  TargetLowering TL(*TM);
+
+  SDLoc Loc;
+  auto IntVT = EVT::getIntegerVT(Context, 8);
+  auto VecVT = EVT::getVectorVT(Context, IntVT, 16, false);
+
+  // Should create BUILD_VECTORs
+  SDValue Val1 = DAG->getConstant(1, Loc, VecVT);
+  SDValue Val2 = DAG->getConstant(3, Loc, VecVT);
+  EXPECT_EQ(Val1->getOpcode(), ISD::BUILD_VECTOR);
+  SDValue Op = DAG->getNode(ISD::ADD, Loc, VecVT, Val1, Val2);
+
+  int SplatIdx = -1;
+  EXPECT_EQ(DAG->getSplatSourceVector(Op, SplatIdx), Op);
+  EXPECT_EQ(SplatIdx, 0);
+}
+
+TEST_F(AArch64SelectionDAGTest, getSplatSourceVector_Scalable_SPLAT_VECTOR) {
+  if (!TM)
+    return;
+
+  TargetLowering TL(*TM);
+
+  SDLoc Loc;
+  auto IntVT = EVT::getIntegerVT(Context, 8);
+  auto VecVT = EVT::getVectorVT(Context, IntVT, 16, true);
+  // Create a SPLAT_VECTOR
+  SDValue Op = DAG->getConstant(1, Loc, VecVT);
+  EXPECT_EQ(Op->getOpcode(), ISD::SPLAT_VECTOR);
+
+  int SplatIdx = -1;
+  EXPECT_EQ(DAG->getSplatSourceVector(Op, SplatIdx), Op);
+  EXPECT_EQ(SplatIdx, 0);
+}
+
+TEST_F(AArch64SelectionDAGTest, getSplatSourceVector_Scalable_ADD_of_SPLAT_VECTOR) {
+  if (!TM)
+    return;
+
+  TargetLowering TL(*TM);
+
+  SDLoc Loc;
+  auto IntVT = EVT::getIntegerVT(Context, 8);
+  auto VecVT = EVT::getVectorVT(Context, IntVT, 16, true);
+
+  // Should create SPLAT_VECTORS
+  SDValue Val1 = DAG->getConstant(1, Loc, VecVT);
+  SDValue Val2 = DAG->getConstant(3, Loc, VecVT);
+  EXPECT_EQ(Val1->getOpcode(), ISD::SPLAT_VECTOR);
+  SDValue Op = DAG->getNode(ISD::ADD, Loc, VecVT, Val1, Val2);
+
+  int SplatIdx = -1;
+  EXPECT_EQ(DAG->getSplatSourceVector(Op, SplatIdx), Op);
+  EXPECT_EQ(SplatIdx, 0);
+}
+
 } // end anonymous namespace