[DAGCombine] Don't fold dependent loads across SELECT_CC.

[ebevhan]

Summary

DAGCombine will try to fold two loads that feed a SELECT
or SELECT_CC after the select, resulting in a select of
an address and a single load after.

If either of the loads depend on the other, this is not
legal as it could introduce cycles. However, it only
checked this if the opcode was a SELECT, and not for a
SELECT_CC.

Unfortunately, the only reproducer I have for this is
for our downstream target. I've tried getting it to trigger
on an upstream one but haven't been successful.

[niravd]

Do you have a test case?

[bjope]

In D52306#1240579, @niravd wrote:

Do you have a test case?

Bevin and I have only been able to reproduce this for an out-of-tree target.

I think the problem is similar to the one fixed in this commit (which is the commit that is introducing the code that Bevin is hoisting out from the if-else):

commit 1c5bf3f429927c31b9ffc1308ea1ada7a58ee1c0
Author: Nadav Rotem <[e-mail]>
Date:   Thu Oct 18 18:06:48 2012 +0000

    In SimplifySelectOps we pulled two loads through a select node despite the fact that one was dependent on the other.
    
    rdar://12513091
        
    git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@166196 91177308-0d34-0410-b5e6-96231b3b80d8

That commit added a test case for the SELECT situation (test/CodeGen/X86/2012-10-18-crash-dagco.ll). But I don't know how to modify that test case into using SELECT_CC instead (and the same goes for out out-of-tree-target reproducer).

Maybe you have some bright ideas on how to for example convert the test above into using SELECT_CC instead of SELECT?
(note that I haven't verifier if that old tc from 2012 still ends up in this part of DAG combine)

[niravd]

Sadly, no clever ideas on my part. If it's not repeatable on any of the in-tree backend let's just commit this now; it's a fixes a clear oversight.

[niravd]

It looks like you may not have commit access. Would you like me to commit this for you?

[ebevhan]

Oh, sorry. I should have mentioned that. That would be nice, thanks!

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (LHS.getOperand(0) != RHS.getOperand(0) ||
         // src value info, don't do the transformation if the memory
         // locations are not in the default address space.
         LLD->getPointerInfo().getAddrSpace() != 0 ||
         RLD->getPointerInfo().getAddrSpace() != 0 ||
         !TLI.isOperationLegalOrCustom(TheSelect->getOpcode(),
                                       LLD->getBasePtr().getValueType()))
       return false;
 
+    // The loads must not depend on one another.
+    if (LLD->isPredecessorOf(RLD) ||
+        RLD->isPredecessorOf(LLD))
+      return false;
+
     // Check that the select condition doesn't reach either load.  If so,
     // folding this will induce a cycle into the DAG.  If not, this is safe to
     // xform, so create a select of the addresses.
     SDValue Addr;
     if (TheSelect->getOpcode() == ISD::SELECT) {
       SDNode *CondNode = TheSelect->getOperand(0).getNode();
       if ((LLD->hasAnyUseOfValue(1) && LLD->isPredecessorOf(CondNode)) ||
           (RLD->hasAnyUseOfValue(1) && RLD->isPredecessorOf(CondNode)))
         return false;
-      // The loads must not depend on one another.
-      if (LLD->isPredecessorOf(RLD) ||
-          RLD->isPredecessorOf(LLD))
-        return false;
+
       Addr = DAG.getSelect(SDLoc(TheSelect),
                            LLD->getBasePtr().getValueType(),
                            TheSelect->getOperand(0), LLD->getBasePtr(),
                            RLD->getBasePtr());
     } else {  // Otherwise SELECT_CC
       SDNode *CondLHS = TheSelect->getOperand(0).getNode();
       SDNode *CondRHS = TheSelect->getOperand(1).getNode();