[PATCH] D148249: [AArch64] Peep SELECT_CC patterns that match smin(a,0) and smax(a,0).

[Dave Green via Phabricator via llvm-commits]

dmgreen accepted this revision.
dmgreen added a comment.
This revision is now accepted and ready to land.

Thanks. LGTM



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:8952
+    if ((CC == ISD::SETGT || CC == ISD::SETLT) && LHS == TVal &&
+        RHSC && RHSC->isZero() && CFVal && CFVal->isZero() &&
+        LHS.getValueType() == RHS.getValueType()) {
----------------
cameron.mcinally wrote:
> dmgreen wrote:
> > Is this checking both operands for zero because they might still be different zeroes?
> I wasn't certain a Constant 0 would share the same SDValue across nodes and already had the ConstantSDNodes from the code above. That was the only motivation. What do you think?
It's a little subtle but there is actually two different zeros, depending on whether the constant is opaque or not. I was originally thinking of suggesting the code just checked for RHS == FVal, but the way you have it sounds better.


================
Comment at: llvm/test/CodeGen/AArch64/min-max-peep.ll:7
+
+; These tests check for @llvm.smax, @llvm.smin peeps.
+
----------------
I would use "peephole" or "combine", as peeps is not something I've seen elsewhere.


================
Comment at: llvm/test/CodeGen/AArch64/min-max-peep.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 2
+; RUN: llc -mtriple=aarch64-eabi %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-ISEL
+; RUN: llc -mtriple=aarch64-eabi %s -o - -mattr=cssc | FileCheck %s --check-prefixes=CHECK,CHECK-ISEL-CSSC
----------------
cameron.mcinally wrote:
> dmgreen wrote:
> > CHECK would never match both +cssc base cases, but the CSSC cases should all be the same between global and sdag, from the look of it. Can they share the same prefix and remove CHECK?
> I'll give that a shot. 
> 
> I copied this pattern from the existing min-max.ll test, but I now see that it only differs for i8, i16, and vectors. This change is restricted to i32 and i64.
Looks good thanks. It may be worth adding i8 and i16 sized too. They should get legalized to i32, so should work the same way,


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

https://reviews.llvm.org/D148249