[PATCH] D120422: [AArch64] Optimize comparison chains

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Do you have any cases where the existing And lowering wasn't already performing the folds that the new method does? I feel like it was already working OK, and it may be better to work on top of it as opposed to writing it from scratch. It doesn't seem to be needed for any of the tests, and this example from the commit message already looks OK: https://godbolt.org/z/Mdf8nj5ae. I'm not sure if it's worth sharing the method between And and Or, but this might be better focussing on the Or code.

> This patch generalizes https://reviews.llvm.org/D118327 to cases where results of comparisons are ORRed together,

Cool, that's great to see. It looks like it will be very useful.

> and where the comparison is performed with CCMP instead of CMP/SUBS

I think the existing PerformANDCSELCombine method already handled that. It only requires one of the two operands to be a SUBS, the other can be anything that sets flags, which it re-uses directly. The SUBS gets converted to a CCMP, the other flag-setting instruction is uses as-is as the input.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14073
+    // AL and NV cannot be inverted
+    if (CC2 == AArch64CC::AL || CC2 == AArch64CC::NV) {
+      return SDValue();
----------------
AL and NV conditions are inverses of one another, but they shouldn't come up, as they would just always pick one of the operands without needing result of the comparison. You could probably change it to an assert, but they shouldn't be generated from anywhere in practice.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14249
 
-  if (SDValue R = PerformANDCSELCombine(N, DAG))
-    return R;
+  if (!DAG.getTargetLoweringInfo().isTypeLegal(VT)) {
+    return SDValue();
----------------
I think this code was fine before. Checking for CSEL will inherently check that the type is legal..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120422