[PATCH] D122968: [AArch64][SelectionDAG] Add target-specific implementation of srem

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13594
+  unsigned Lg2 = Divisor.countTrailingZeros();
+  if (Lg2 == 0 || Lg2 == (VT.getScalarSizeInBits() - 1))
+    return SDValue();
----------------
Probably worth a comment explaining this.  Lg2 == 0 folds to a constant.  Not obvious why we're special-casing `Lg2 == (VT.getScalarSizeInBits() - 1)`; does this sequence not work somehow?


================
Comment at: llvm/test/CodeGen/AArch64/srem-lkk.ll:171
+}
+
----------------
Maybe we also want some tests for i16?

Please post a separate patch to add the new tests, so it's easier to compare the impact.  And actually, while you're at it, maybe stick all the power-of-two tests in a separate file with a better name.


================
Comment at: llvm/test/CodeGen/AArch64/srem-vector-lkk.ll:188
+; CHECK-NEXT:    mov v0.h[3], w8
+; CHECK-NEXT:    // kill: def $d0 killed $d0 killed $q0
 ; CHECK-NEXT:    ret
----------------
I think this somehow ended up shorter with D122829?  That might be fine, though; it looks like this version avoids the expensive add-with-shift instructions.


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

https://reviews.llvm.org/D122968