Be more conservative in canonicalizing loads that only store

[stewartd]

Summary

Only canonicalize loads that only store if the store's pointer operand is not reloaded elsewhere. It is common that the demotion of phis to loads/stores will create a number of loads that only store. However, the same location is loaded from in multiple locations. In this case, canonicalizing the loads to integers may not be the best thing to do, as eventually the loaded value will be used as it's original type, forcing bitcasts to be spread throughout the code.

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

   return NewStore;
 }
 
+/// \brief Determine whether a Load which only Stores should be canonicalized
+/// to an integer type.
+///
+/// If a load which is only stored is not loaded elsewhere, then we assume
+/// it is advantageous to canonicalize. However, if it is loaded elsewhere,
+/// then the assumption is that it should stay that type in order to avoid
+/// unnecessary bitcasts later.
+static bool shouldCanonicalizeLoadStore(LoadInst &LI) {
+  bool canonicalize = true;
+
+  // Check if this load is only used to store.
+  // If true, then check if it's pointer operand is used elsewhere.
+  // Only if the pointer operand is not used elsewhere is it canonicalized.
+  SmallVector<Instruction *, 4> WorkList;
+
+  for (User *U : LI.users()) {
+    auto *SI = dyn_cast<StoreInst>(U);
+    if (!(SI && SI->getPointerOperand() != &LI)) {
+      // This load does not solely store.
+      canonicalize = false;
+      break;
+    }
+
+    // This load only stores. Now push all uses of the pointer operand
+    // onto the worklist to be checked later.
+    Value *SPO = SI->getPointerOperand();
+
+    if (SPO) {
+      for (User *user : SPO->users()) {
+        Instruction *I = dyn_cast<Instruction>(user);
+        if (I) {
+          WorkList.push_back(I);
+        }
+      }
+    }
+  }
+
+  // Check all instructions on the worklist to ensure they are all
+  // store operations.
+  while(canonicalize && !WorkList.empty()) {
+    Instruction *I = WorkList.pop_back_val();
+    auto *SI = dyn_cast<StoreInst>(I);
+    if (!SI) {
+      canonicalize = false;
+    }
+  }
+
+  return canonicalize;
+}
+
 /// \brief Combine loads to match the type of value their uses after looking
 /// through intervening bitcasts.
 ///

   if (!Ty->isIntegerTy() && Ty->isSized() &&
       DL.isLegalInteger(DL.getTypeStoreSizeInBits(Ty)) &&
       DL.getTypeStoreSizeInBits(Ty) == DL.getTypeSizeInBits(Ty)) {
-    if (std::all_of(LI.user_begin(), LI.user_end(), [&LI](User *U) {
-          auto *SI = dyn_cast<StoreInst>(U);
-          return SI && SI->getPointerOperand() != &LI;
-        })) {
+    if (shouldCanonicalizeLoadStore(LI)) {
       LoadInst *NewLoad = combineLoadToNewType(
           IC, LI,
           Type::getIntNTy(LI.getContext(), DL.getTypeStoreSizeInBits(Ty)));

test/Transforms/InstCombine/load.ll

 
   ret void
 }
+
+define void @test18(float* %x, float* %a, float* %b) {
+; Check that loads that only store do not get canonicalized if the
+; pointer operand of the store is reloaded.
+;
+; CHECK-LABEL: @test18(
+; CHECK: %[[L:.*]] = load float, float*
+; CHECK: store float %[[L]]
+
+entry:
+  %x1 = load float, float* %x
+  store float %x1, float* %a
+
+  %y = load float, float* %a
+  %z = load float, float* %b
+
+  %yz.sub = fsub float %y, %z
+  store float %yz.sub, float* %b
+
+  ret void
+}