[PATCH] D89346: [SelectionDAG][X86] Enable SimplifySetCC CTPOP transforms for vector splats

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a reviewer: bkramer.
spatel added a comment.

For reference, the original transform was added with rG123621:
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20110117/115345.html
It was intentionally not checking for legality on scalars because that was better for the x86 motivating case, but I'm not sure if that still holds after 9+ years. It seems unlikely that holds across other targets.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:3444-3445
+
+  if (auto *N1C = isConstOrConstSplat(N1)) {
+    const APInt &C1 = N1C->getAPIntValue();
 
----------------
I think this blob of ctpop transforms is big enough to make into a helper function as a preliminary cleanup, and that + early exits might make it easier to understand/change the logic. 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:3453
+        N0.getScalarValueSizeInBits() >
+            Log2_32(N0.getOperand(0).getScalarValueSizeInBits()))
       CTPOP = N0.getOperand(0);
----------------
Is there any way to expose the Log2 difference in a test?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:3464
+           (Cond == ISD::SETUGT && C1 == 1)) &&
+          (!VT.isVector() || !isOperationLegal(ISD::CTPOP, CTVT))) {
         SDValue NegOne = DAG.getAllOnesConstant(dl, CTVT);
----------------
It's hard to follow the vector vs. legal logic diffs here and the next set of folds. Make bools with explanatory names for these and/or add code comments to describe what we are allowing/restricting and why?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89346