[PATCH] D106561: [AArch64] Optimise min/max lowering in ISel

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:7107
+
+  if ((VT != MVT::v1i64 && VT != MVT::v2i64) ||
+      useSVEForFixedLengthVectorVT(VT, /*OverrideNEON=*/true)) {
----------------
I think you can move this if block to the start of the function, i.e.

  unsigned Opcode = Op.getOpcode();
  if (...) {
    switch (Opcode) {
    ...
    }
  }


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:7108
+  if ((VT != MVT::v1i64 && VT != MVT::v2i64) ||
+      useSVEForFixedLengthVectorVT(VT, /*OverrideNEON=*/true)) {
+    switch (Opcode) {
----------------
I think you might be able to just do this:

  if (useSVEForFixedLengthVectorVT(VT, /*OverrideNEON=*/true)) {

instead?


================
Comment at: llvm/test/Analysis/CostModel/AArch64/min-max.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py
 ; RUN: opt < %s -mtriple=aarch64--linux-gnu -cost-model -analyze | FileCheck %s --check-prefix=COST
----------------
nit: It might be easier to review the patch if you first introduced a NFC change to update the tests using utils/update_analyze_test_checks.py?


================
Comment at: llvm/test/CodeGen/AArch64/vecreduce-umax-legalization.ll:90
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:   mov     x8, v0.d[1]
-; CHECK-NEXT:   fmov    x9, d0
-; CHECK-NEXT:   cmp     x9, x8
-; CHECK-NEXT:   csel    x0, x9, x8, hi
-; CHECK-NEXT:   ret
+; CHECK-NEXT:    ext v1.16b, v0.16b, v0.16b, #8
+; CHECK-NEXT:    cmhi d2, d0, d1
----------------
Hi @Rin, at first glance this code looks like it might be worse than before? I realise the instruction count is the same, but I wonder if the cost of 'ext' might be higher?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106561