[PATCH] D79083: [CodeGen] Fix warnings due to SelectionDAG::getSplatSourceVector

Fri May 1 17:17:05 PDT 2020

fpetrogalli added a comment.

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

================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:2279
 /// isSplatValue - Return true if the vector V has the same value
 /// across all DemandedElts.
 bool SelectionDAG::isSplatValue(SDValue V, const APInt &DemandedElts,
----------------
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! :)

================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:2285-2286

+  if (!VT.isScalableVector() && !DemandedElts)
+    return false; // No demanded elts, better to assume we don't know anything.
+
----------------
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");
```

================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:2389

 SDValue SelectionDAG::getSplatSourceVector(SDValue V, int &SplatIdx) {
   V = peekThroughExtractSubvectors(V);
----------------
This seems also a good candidate to unit test, at least for the three cases you have modified?

================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:202

+TEST_F(AArch64SelectionDAGTest, isSplatValue_Fixed_BUILD_VECTOR) {
+  if (!TM)
----------------
The tests would benefit of a couple of lines of comment explaining what you are testing...

================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:210
+  auto IntVT = EVT::getIntegerVT(Context, 8);
+  auto VecVT = EVT::getVectorVT(Context, IntVT, 16, false);
+  auto Op = DAG->getConstant(1, Loc, VecVT);
----------------
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.

================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:211
+  auto VecVT = EVT::getVectorVT(Context, IntVT, 16, false);
+  auto Op = DAG->getConstant(1, Loc, VecVT);
+  EXPECT_EQ(Op->getOpcode(), ISD::BUILD_VECTOR);
----------------
Avoid `auto` when the type is not explicit in the RHS.

================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:213
+  EXPECT_EQ(Op->getOpcode(), ISD::BUILD_VECTOR);
+  EXPECT_EQ(DAG->isSplatValue(Op, false), true);
+
----------------
`EXPECT_TRUE(DAG->isSplatValue(Op, false))`

================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:217
+  APInt DemandedElts;
+  EXPECT_EQ(DAG->isSplatValue(Op, DemandedElts, UndefElts), false);
+
----------------
Similarly, use `EXPECT_FALSE`

================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:220
+  DemandedElts = APInt(16, 3);
+  EXPECT_EQ(DAG->isSplatValue(Op, DemandedElts, UndefElts), true);
+}
----------------
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? 

================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:234-235
+  // Should create BUILD_VECTORs
+  auto Val1 = DAG->getConstant(1, Loc, VecVT);
+  auto Val2 = DAG->getConstant(3, Loc, VecVT);
+  EXPECT_EQ(Val1->getOpcode(), ISD::BUILD_VECTOR);
----------------
Please don't use `auto` here.

================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:239
+
+  EXPECT_EQ(DAG->isSplatValue(Op, false), true);
+
----------------
`EXPECT_TRUE`

================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:243
+  APInt DemandedElts;
+  EXPECT_EQ(DAG->isSplatValue(Op, DemandedElts, UndefElts), false);
+
----------------
`EXPECT_FALSE`

================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:246
+  DemandedElts = APInt(16, 3);
+  EXPECT_EQ(DAG->isSplatValue(Op, DemandedElts, UndefElts), true);
+}
----------------
`EXPECT_TRUE`

================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:258
+  auto VecVT = EVT::getVectorVT(Context, IntVT, 16, true);
+  auto Op = DAG->getConstant(1, Loc, VecVT);
+  EXPECT_EQ(Op->getOpcode(), ISD::SPLAT_VECTOR);
----------------
Don't use `auto` here.

================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:260
+  EXPECT_EQ(Op->getOpcode(), ISD::SPLAT_VECTOR);
+  EXPECT_EQ(DAG->isSplatValue(Op, false), true);
+
----------------
`EXPECT_TRUE`

================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:264
+  APInt DemandedElts;
+  EXPECT_EQ(DAG->isSplatValue(Op, DemandedElts, UndefElts), true);
+
----------------
`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.

================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:266
+
+  // These bits should be ignored.
+  DemandedElts = APInt(16, 3);
----------------
Yeah, so, why do you always use `APInt(16, 3)`? Can you vary those values?

================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:282-283
+  // Should create SPLAT_VECTORS
+  auto Val1 = DAG->getConstant(1, Loc, VecVT);
+  auto Val2 = DAG->getConstant(3, Loc, VecVT);
+  EXPECT_EQ(Val1->getOpcode(), ISD::SPLAT_VECTOR);
----------------
Don't use `auto`.

================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:285
+  EXPECT_EQ(Val1->getOpcode(), ISD::SPLAT_VECTOR);
+  auto Op = DAG->getNode(ISD::ADD, Loc, VecVT, Val1, Val2);
+
----------------
Don't use `auto`.

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

https://reviews.llvm.org/D79083