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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a reviewer: lebedev.ri.
lebedev.ri added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:908
+  // add(add(ext, 1), ext)
+  // add(ext, add(ext, 1))
+  SDValue Add = Op.getOperand(0);
----------------
There's also the case where one of the operands is a constant, so you don't have an explicit +1


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:947-950
+    if (ExtOp == ISD::ZERO_EXTEND)
+      return true;
+    if (ExtOp != ISD::SIGN_EXTEND)
+      return false;
----------------
Since you have already avoided the other misdesign (by starting from the shift and not trunc),
can we also avoid one here, by looking at the known sign/leading zero bits instead of an explicit extension?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:960-961
+
+  // Is the result of the right shift being truncated to the same value type as
+  // the original operands, OpA and OpB?
+  SDValue OpA = ExtOpA.getOperand(0);
----------------
Don't you need to check that the addition is no-wrap,
i.e. don't you need to check how many bits were added during extension?

Right now this seems like a miscompile, one that will be avoided
if you use known bits instead of looking for extensions.


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

https://reviews.llvm.org/D106237