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

Mon May 4 00:29:33 PDT 2020

david-arm marked 13 inline comments as done.
david-arm added inline comments.

================
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,
----------------
fpetrogalli wrote:
> 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! :)
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.

================
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.
+
----------------
fpetrogalli wrote:
> 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");
> ```
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.

================
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.
+
----------------
david-arm wrote:
> fpetrogalli wrote:
> > 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");
> > ```
> 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.
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.

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

 SDValue SelectionDAG::getSplatSourceVector(SDValue V, int &SplatIdx) {
   V = peekThroughExtractSubvectors(V);
----------------
fpetrogalli wrote:
> This seems also a good candidate to unit test, at least for the three cases you have modified?
Sure, I'd forgotten to test this. Sigh, writing unit tests is way more time consuming than the code changes themselves. :)

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

+TEST_F(AArch64SelectionDAGTest, isSplatValue_Fixed_BUILD_VECTOR) {
+  if (!TM)
----------------
fpetrogalli wrote:
> The tests would benefit of a couple of lines of comment explaining what you are testing...
Fair point! I'll add comments.

================
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);
----------------
fpetrogalli wrote:
> 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.
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?

================
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);
----------------
fpetrogalli wrote:
> Avoid `auto` when the type is not explicit in the RHS.
See my respone to the last 'auto' comment.

================
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);
----------------
fpetrogalli wrote:
> Please don't use `auto` here.
See my respone to the last 'auto' comment.

================
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);
----------------
fpetrogalli wrote:
> Don't use `auto` here.
See my respone to the last 'auto' comment.

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

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

================
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);
----------------
fpetrogalli wrote:
> Don't use `auto`.
See my respone to the last 'auto' comment.

================
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);
+
----------------
fpetrogalli wrote:
> Don't use `auto`.
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.

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

https://reviews.llvm.org/D79083