[PATCH] D104042: [AArch64] Improve SAD pattern

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Nice patch. It's a big combine, but we certainly have bigger elsewhere. Sometimes it's possible to do things like this as smaller pieces, that add up to the same thing. Speaking of which, would it be possible to combine (in a separate patch, maybe as a tablegen pattern) addv (uaddlp x) -> uaddlv x ?

Would it make sense to add signed handling too? If I'm not mistaken, if it has sext's as opposed to zext's, it can turn into addv(uaddlp(sabd,sabd2). The sadb's produce an unsigned value from signed inputs, so the remaining extensions are zext's not sexts and it doesn't need a SADDLP node. That may not be correct though, so, um, make sure you check it.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12004
+//         v8i8 UABD high8:v16i8 a, high8:v16i8 b
+static SDValue performVecReduceAddCombineWithUADALP(SDNode *N,
+                                                    SelectionDAG &DAG) {
----------------
This doesn't appear to use UADALP. Is that meant as a shorthand for UADA + UADDLP?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12011
+  SDValue VecReduceOp0 = N->getOperand(0);
+  unsigned Opcode = VecReduceOp0.getOpcode();
+  // Assumed v16i32 abs
----------------
This is mostly a style comment, so feel free to ignore if you like. They Opcode's are only used once in the checks, so could be done inline without the variable. Having the "ABSOp0" and "SUB" nodes could just be one variable called "SUB" too, as we are checking the opcode on them straight away.


================
Comment at: llvm/test/CodeGen/AArch64/neon-sad.ll:7
+
+define i32 @test_sad_v16i8(i8* nocapture readonly %a, i8* nocapture readonly %b) {
+; CHECK-LABEL: test_sad_v16i8:
----------------
It can be good to pre-commit the tests, to just show the differences in the review. It makes it easier to see what the patch does.


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

https://reviews.llvm.org/D104042