[PATCH] D106237: [ISel] Port AArch64 HADD and RHADD to ISel

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

> Providing that the depth is limited, is there an issue with doing it as a demand bits combine?

My main reason for questioning was because as far as I know this is a larger transform than anything else we do in SimplifiedDemandedBits. So it seemed a bit of a new direction for SimplifiedDemandedBits, but if we're not concerned about that then I don't object to this patch.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:953
+  unsigned NumZeroA =
+      DAG.computeKnownBits(ExtOpA, Depth).countMinLeadingZeros();
+  unsigned NumZeroB =
----------------
I'm not sure, but would it be better to use countMaxActiveBits and ComputeMaxSignificantBits. And do all the comparisons against the scalar bit width instead of against 1 and 2.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:1000
+  if (VT.isVector())
+    NVT = VT.changeVectorElementType(NVT);
+  if (!TLI.isOperationLegalOrCustom(AVGOpc, NVT))
----------------
changeVectorElementType is a little broken. If the VT is an MVT, but the VT with the element type replaced isn't an MVT, the function will assert because it needs an LLVMContext to create an EVT which it can't get from the MVT.

Maybe it's ok in this case as long as our MVTs always have all power of 2 element types smaller for each MVT. Like MVT::vXi32 would need MVT::vXi16 and MVT::vXi8 to exist.


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

https://reviews.llvm.org/D106237