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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/ISDOpcodes.h:614
 
+  /// HADDS/HADDU - Having add - Add two integers using an integer of type
+  /// i[N+1], halving the result by shifting it one bit right.
----------------
efriedma wrote:
> RKSimon wrote:
> > having add?
> > Maybe add a code snippet?
> There's really very little incentive to keep ISD opcode names short, particularly rarely used ones like this.  I'd prefer to spell it out (particularly the "h"; for example, x86 has "haddps", where the "h" stands for "horizontal").
> 
> Maybe ADD_HALVE_UNSIGNED_ROUND_DOWN and ADD_HALVE_SIGNED_ROUND_UP?
Halide calls these halving_add and rounding_halving_add.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:935
+  // The two extends are sext and the shift is a sra.
+  // The two extends are zext and the shift is srl and the top bit is not
+  // demanded.
----------------
Is zext supposed to be sext?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:945
+      return true;
+    return DemandedBits.countLeadingZeros() >= 1;
+  };
----------------
Can this be DemandedBits.isPositive() or DemandedBits.isSignBitClear()? Counting bits seems like overkill.


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

https://reviews.llvm.org/D106237