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

[Dave Green via Phabricator via llvm-commits]

dmgreen 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);
----------------
lebedev.ri wrote:
> There's also the case where one of the operands is a constant, so you don't have an explicit +1
For ARM/Arch64 that would be an hadd (avgfloor), IIUC. For X86 where avgceil is legal but avgfloor is not, we can probably do something about that if needed, or make it target specific. It's probably best not to try and address ever issue in this single patch though.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:947-950
+    if (ExtOp == ISD::ZERO_EXTEND)
+      return true;
+    if (ExtOp != ISD::SIGN_EXTEND)
+      return false;
----------------
lebedev.ri wrote:
> 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?
Sure, sounds good. I've given it a go, but it might be a bit messy. We have to get a type to use for the legalization, which was previously just using the extend type. Let me know if you have any suggestions.


================
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);
----------------
lebedev.ri wrote:
> 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.
MVE already has some nowrap tablegen patterns, as they don't require any extends and can be recognised from a add+shift.

I've not added anything with nowrap here (maybe we can extend this in a future patch, so long as this is correct so far). The other patterns mostly verify with a single bit of extension, so long as I have that right, and I've added checks where they seem to need 2.


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

https://reviews.llvm.org/D106237