[PATCH] D53236: [SelectionDAG] swap select_cc operands to enable folding

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D53236#1275285, @labrinea wrote:

> In https://reviews.llvm.org/D53236#1265594, @spatel wrote:
>
> > Is the motivating case integer or FP?
> >  I'm asking because we have a canonicalization for integer cmp+sel for the IR in these tests, but we're missing the corresponding FP transform. 
> >  If we add the FP canonicalization in IR, would there still be a need for this backend patch? Ie, is something generating this select code in the DAG itself?
>
>
> Yes, the motivating example was floating point comparison. Indeed, I added the unordered predicates and modified InstCombine to handle fpcmp. It then swapped the operands and inverted the predicates for me. But what does "Canonical" actually mean? The backend still won't be able to do the transformation of the description for `select_cc(x, y, 0, 16, cc)` if cc is already canonical, assuming this is a reachable state in DAG.


Yes, we can't guarantee that this pattern won't appear in the DAG if the cc was already the canonical predicate. We could try harder in instcombine to make the larger constant appear as the 'true value' for the select, but we can't guarantee it (if there are extra uses of the compare, then we won't do that transform in IR and increase the instruction count).



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:18287
+    if (Swap) {
+      CC = ISD::getSetCCInverse(CC, N0.getValueType().isInteger());
+      std::swap(N2C, N3C);
----------------
I don't know how to expose this as a bug, but this scares me. We are changing the value of CC without swapping the N2/N3 values, and it's possible to fall-through to the code underneath this block. At that point, we have the wrong CC value.

Please move this block of code into a helper function as a preliminary step, so we don't have this risk.


================
Comment at: test/CodeGen/AArch64/select_cc.ll:1-4
+; RUN: llc < %s -mtriple=arm64 | FileCheck %s
+
+; CHECK_LABEL: select_ogt_float
+; CHECK:       fcmp s0, s1
----------------
Please add this test file to trunk as a preliminary step and use utils/update_llc_test_checks.py to auto-generate the check lines.


https://reviews.llvm.org/D53236