[PATCH] D120422: [AArch64] Optimize comparison chains

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.
Herald added a project: All.

Thanks for the updates. Looks like a good patch, if we can clean up the details a little.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14023
+  SDValue CSel1 = N->getOperand(1);
+  bool IsAnd = N->getOpcode() == ISD::AND;
+
----------------
This is only used in one place? If so it might be simpler inline, or at least closer to the use.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14079
                                 const AArch64Subtarget *Subtarget) {
   // Attempt to form an EXTR from (or (shl VAL1, #N), (srl VAL2, #RegWidth-N))
   SelectionDAG &DAG = DCI.DAG;
----------------
Can you move this comment next to tryCombineToEXTR whilst you are here too?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14089
 
+  if (SDValue Res = performANDORCSELCombine(N, DAG)) {
+    return Res;
----------------
This can be removed?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14202
 
-  if (!VT.isVector() || !DAG.getTargetLoweringInfo().isTypeLegal(VT))
     return SDValue();
----------------
Leave this in, I would think?


================
Comment at: llvm/test/CodeGen/AArch64/arm64-ccmp.ll:760-761
 
 ; Should not use ccmp if we have to compute the or expression in an integer
 ; register anyway because of other users.
 define i64 @select_noccmp2(i64 %v1, i64 %v2, i64 %v3, i64 %r) {
----------------
I think this comment can be removed now. The code looks fine.


================
Comment at: llvm/test/CodeGen/AArch64/cmp-chains.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=aarch64-- | FileCheck %s
----------------
It can be good to pre-commit the tests, so just the differences get shown in the review. It makes it easier to see what changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120422