[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

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! :)

2285–2286

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");

2378

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

llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp

202 ↗

(On Diff #261442)

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

210 ↗

(On Diff #261442)

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 ↗

(On Diff #261442)

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

213 ↗

(On Diff #261442)

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

217 ↗

(On Diff #261442)

Similarly, use EXPECT_FALSE

220 ↗

(On Diff #261442)

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 ↗

(On Diff #261442)

Please don't use auto here.

239 ↗

(On Diff #261442)

EXPECT_TRUE

243 ↗

(On Diff #261442)

EXPECT_FALSE

246 ↗

(On Diff #261442)

EXPECT_TRUE

258 ↗

(On Diff #261442)

Don't use auto here.

260 ↗

(On Diff #261442)

EXPECT_TRUE

264 ↗

(On Diff #261442)

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 ↗

(On Diff #261442)

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

282–283 ↗

(On Diff #261442)

Don't use auto.

285 ↗

(On Diff #261442)

Don't use auto.

[david-arm]

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

2279

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.

2285–2286

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.

2285–2286

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.

2378

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 ↗

(On Diff #261442)

Fair point! I'll add comments.

210 ↗

(On Diff #261442)

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 ↗

(On Diff #261442)

See my respone to the last 'auto' comment.

234–235 ↗

(On Diff #261442)

See my respone to the last 'auto' comment.

258 ↗

(On Diff #261442)

See my respone to the last 'auto' comment.

264 ↗

(On Diff #261442)

OK sure, seems reasonable.

266 ↗

(On Diff #261442)

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 ↗

(On Diff #261442)

See my respone to the last 'auto' comment.

285 ↗

(On Diff #261442)

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 ↗

(On Diff #261442)

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 ↗

(On Diff #261442)

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 ↗

(On Diff #261442)

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.
 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 (V.getOpcode() == ISD::SPLAT_VECTOR)
+    return true;
+
+  if (!DemandedElts || VT.isScalableVector())
+    return false; // No demanded elts, better to assume we don't know anything.
+
   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) {
bool SelectionDAG::isSplatValue(SDValue V, const APInt &DemandedElts,
 
   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();
+  // For now we don't support this with scalable vectors.
+  unsigned NumElts = VT.isScalableVector() ? 0 : VT.getVectorNumElements();
 
   APInt UndefElts;
   APInt DemandedElts = APInt::getAllOnesValue(NumElts);
   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: {
+    if (VT.isScalableVector())
+      return SDValue();
+
     APInt UndefElts;
     APInt DemandedElts = APInt::getAllOnesValue(VT.getVectorNumElements());
     if (isSplatValue(V, DemandedElts, UndefElts)) {
       // 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:
+    return V;
   case ISD::VECTOR_SHUFFLE: {
     // 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();