[InstCombine] Fix PR35618: Instcombine hangs on single minmax load bitcast.

[ABataev]

Summary

If we have pattern `store (load(bitcast(select (cmp(V1, V2), &V1,
&V2)))), bitcast)`, but the load is used in other instructions, it leads
to looping in InstCombiner. Patch adds additional check that all users
of the load instructions are stores and then replaces all uses of load
instruction by the new one with new type.

[RKSimon]

Isn't that PR35618? Typo in subject line.

test/Transforms/InstCombine/multiple-uses-load-bitcast-select.ll

6 ↗

(On Diff #126384)

Do you need this test given that @spatel reduced it to the one below?

[ABataev]

In D41072#951214, @RKSimon wrote:

Isn't that PR35618? Typo in subject line.

Yep, missed the digit

test/Transforms/InstCombine/multiple-uses-load-bitcast-select.ll

6 ↗

(On Diff #126384)

I can remove it, sure

[ABataev]

Removed extra test

[spatel]

LGTM. Since the bitcast-creating canonicalization that we're trying to inhibit can affect multiple stores, we have to handle that case too.

llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

bool removeBitcastsFromLoadStoreOnMinMax(InstCombiner &IC, StoreInst &SI) {
   if (!match(SI.getValueOperand(), m_Load(m_BitCast(m_Value(LoadAddr)))))
     return false;
   auto *LI = cast<LoadInst>(SI.getValueOperand());
   if (!LI->getType()->isIntegerTy())
     return false;
   if (!isMinMaxWithLoads(LoadAddr))
     return false;
 
+  if (!all_of(LI->users(), [LI](User *U) {
+        auto *SI = dyn_cast<StoreInst>(U);
+        return SI && SI->getPointerOperand() != LI &&
+               !SI->getPointerOperand()->isSwiftError();
+      }))
+    return false;
+
+  IC.Builder.SetInsertPoint(LI);
   LoadInst *NewLI = combineLoadToNewType(
       IC, *LI, LoadAddr->getType()->getPointerElementType());
-  combineStoreToNewValue(IC, SI, NewLI);
+  // Replace all the stores with stores of the newly loaded value.
+  for (auto *UI : LI->users()) {
+    auto *SI = cast<StoreInst>(UI);
+    IC.Builder.SetInsertPoint(SI);
+    combineStoreToNewValue(IC, *SI, NewLI);
+    IC.eraseInstFromFunction(*SI);
+  }
+  IC.eraseInstFromFunction(*LI);
   return true;
 }
 
 Instruction *InstCombiner::visitStoreInst(StoreInst &SI) {
   Value *Val = SI.getOperand(0);
   Value *Ptr = SI.getOperand(1);
 
   // Try to canonicalize the stored type.
Instruction *InstCombiner::visitStoreInst(StoreInst &SI) {
   else if (StoreAlign == 0)
     SI.setAlignment(EffectiveStoreAlign);
 
   // Try to canonicalize the stored type.
   if (unpackStoreToAggregate(*this, SI))
     return eraseInstFromFunction(SI);
 
   if (removeBitcastsFromLoadStoreOnMinMax(*this, SI))
-    return eraseInstFromFunction(SI);
+    return nullptr;
 
   // Replace GEP indices if possible.
   if (Instruction *NewGEPI = replaceGEPIdxWithZero(*this, Ptr, SI)) {
       Worklist.Add(NewGEPI);
       return &SI;
   }
 
   // Don't hack volatile/ordered stores.

llvm/trunk/test/Transforms/InstCombine/multiple-uses-load-bitcast-select.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -instcombine -S -data-layout="E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-v128:64-a:8:16-n32:64" | FileCheck %s
+
+define void @PR35618(i64* %st1, double* %st2) {
+; CHECK-LABEL: @PR35618(
+; CHECK-NEXT:    [[Y1:%.*]] = alloca double, align 8
+; CHECK-NEXT:    [[Z1:%.*]] = alloca double, align 8
+; CHECK-NEXT:    [[LD1:%.*]] = load double, double* [[Y1]], align 8
+; CHECK-NEXT:    [[LD2:%.*]] = load double, double* [[Z1]], align 8
+; CHECK-NEXT:    [[TMP10:%.*]] = fcmp olt double [[LD1]], [[LD2]]
+; CHECK-NEXT:    [[TMP121:%.*]] = select i1 [[TMP10]], double [[LD1]], double [[LD2]]
+; CHECK-NEXT:    [[TMP1:%.*]] = bitcast i64* [[ST1:%.*]] to double*
+; CHECK-NEXT:    store double [[TMP121]], double* [[TMP1]], align 8
+; CHECK-NEXT:    store double [[TMP121]], double* [[ST2:%.*]], align 8
+; CHECK-NEXT:    ret void
+;
+  %y1 = alloca double
+  %z1 = alloca double
+  %ld1 = load double, double* %y1
+  %ld2 = load double, double* %z1
+  %tmp10 = fcmp olt double %ld1, %ld2
+  %sel = select i1 %tmp10, double* %y1, double* %z1
+  %tmp11 = bitcast double* %sel to i64*
+  %tmp12 = load i64, i64* %tmp11
+  store i64 %tmp12, i64* %st1
+  %bc = bitcast double* %st2 to i64*
+  store i64 %tmp12, i64* %bc
+  ret void
+}
+