[GVN] non-functional code movement

[vtjnash]

Summary

In preparation for later fixes to the non-integral addrspace handling (D59661)

[reames]

LGTM w/one required change.

lib/Transforms/Utils/VNCoercion.cpp

314 ↗

(On Diff #191976)

Remove this line, not NFC

[vtjnash]

lib/Transforms/Utils/VNCoercion.cpp

314 ↗

(On Diff #191976)

As far as I could tell, all later code paths are guarded by the full condition that I'm now also using here:

https://github.com/llvm-mirror/llvm/blob/7b016165658561a2ec5bdffea134fe50cc9bcf7a/lib/Analysis/ConstantFolding.cpp#L538
https://github.com/llvm-mirror/llvm/blob/7b016165658561a2ec5bdffea134fe50cc9bcf7a/lib/Analysis/ConstantFolding.cpp#L669
https://github.com/llvm-mirror/llvm/blob/7b016165658561a2ec5bdffea134fe50cc9bcf7a/lib/Analysis/ConstantFolding.cpp#L620
https://github.com/llvm-mirror/llvm/blob/7b016165658561a2ec5bdffea134fe50cc9bcf7a/lib/Analysis/ConstantFolding.cpp#L606

I thought it seemed odd to check only part of the condition early, while deferring the full check until later.

331 ↗

(On Diff #191976)

OTOH, this might not be strictly NFC, since, while not a semantic difference in IR, I thought it seemed like ConstantFoldLoadFromConstPtr may sometimes have difficultly looking through this BitCast.

[vtjnash]

remove change that could result in a functional difference

[loladiro]

@vtjnash Was the review comment addressed? If so I assume you want me to land this, since you don't have commit?

[vtjnash]

lib/Transforms/Utils/VNCoercion.cpp

314 ↗

(On Diff #191976)

Bump. I wanted to confirm you agree now that this part of the change is NFC. I think I've addressed your other comments, so the other commits in the stack should also be ready for re-review also. Thanks!

[vtjnash]

@loladiro can you land this for me?

[loladiro]

lib/Transforms/Utils/VNCoercion.cpp

314 ↗

(On Diff #191976)

I've looked at this and I think I concur with this analysis, so I'll go ahead and land this, since @reames was ok with the rest of it. @reames If we both missed something, please do let me know and I'll be happy to revert or adjust.

llvm/trunk/lib/Transforms/Scalar/GVN.cpp

 bool GVN::AnalyzeLoadAvailability(LoadInst *LI, MemDepResult DepInfo,
                                   Value *Address, AvailableValue &Res) {
   assert((DepInfo.isDef() || DepInfo.isClobber()) &&
          "expected a local dependence");
   assert(LI->isUnordered() && "rules below are incorrect for ordered access");
 
   const DataLayout &DL = LI->getModule()->getDataLayout();
 
+  Instruction *DepInst = DepInfo.getInst();
   if (DepInfo.isClobber()) {
     // If the dependence is to a store that writes to a superset of the bits
     // read by the load, we can extract the bits we need for the load from the
     // stored value.
-    if (StoreInst *DepSI = dyn_cast<StoreInst>(DepInfo.getInst())) {
+    if (StoreInst *DepSI = dyn_cast<StoreInst>(DepInst)) {
       // Can't forward from non-atomic to atomic without violating memory model.
       if (Address && LI->isAtomic() <= DepSI->isAtomic()) {
         int Offset =
           analyzeLoadFromClobberingStore(LI->getType(), Address, DepSI, DL);
         if (Offset != -1) {
           Res = AvailableValue::get(DepSI->getValueOperand(), Offset);
           return true;
         }
       }
     }
 
     // Check to see if we have something like this:
     //    load i32* P
     //    load i8* (P+1)
     // if we have this, replace the later with an extraction from the former.
-    if (LoadInst *DepLI = dyn_cast<LoadInst>(DepInfo.getInst())) {
+    if (LoadInst *DepLI = dyn_cast<LoadInst>(DepInst)) {
       // If this is a clobber and L is the first instruction in its block, then
       // we have the first instruction in the entry block.
       // Can't forward from non-atomic to atomic without violating memory model.
       if (DepLI != LI && Address && LI->isAtomic() <= DepLI->isAtomic()) {
         int Offset =
           analyzeLoadFromClobberingLoad(LI->getType(), Address, DepLI, DL);
 
         if (Offset != -1) {
           Res = AvailableValue::getLoad(DepLI, Offset);
           return true;
         }
       }
     }
 
     // If the clobbering value is a memset/memcpy/memmove, see if we can
     // forward a value on from it.
-    if (MemIntrinsic *DepMI = dyn_cast<MemIntrinsic>(DepInfo.getInst())) {
+    if (MemIntrinsic *DepMI = dyn_cast<MemIntrinsic>(DepInst)) {
       if (Address && !LI->isAtomic()) {
         int Offset = analyzeLoadFromClobberingMemInst(LI->getType(), Address,
                                                       DepMI, DL);
         if (Offset != -1) {
           Res = AvailableValue::getMI(DepMI, Offset);
           return true;
         }
       }
     }
     // Nothing known about this clobber, have to be conservative
     LLVM_DEBUG(
         // fast print dep, using operator<< on instruction is too slow.
         dbgs() << "GVN: load "; LI->printAsOperand(dbgs());
-        Instruction *I = DepInfo.getInst();
-        dbgs() << " is clobbered by " << *I << '\n';);
+        dbgs() << " is clobbered by " << *DepInst << '\n';);
     if (ORE->allowExtraAnalysis(DEBUG_TYPE))
       reportMayClobberedLoad(LI, DepInfo, DT, ORE);
 
     return false;
   }
   assert(DepInfo.isDef() && "follows from above");
 
-  Instruction *DepInst = DepInfo.getInst();
-
   // Loading the allocation -> undef.
   if (isa<AllocaInst>(DepInst) || isMallocLikeFn(DepInst, TLI) ||
       // Loading immediately after lifetime begin -> undef.
       isLifetimeStart(DepInst)) {
     Res = AvailableValue::get(UndefValue::get(LI->getType()));
     return true;
   }
 
   // Loading from calloc (which zero initializes memory) -> zero
   if (isCallocLikeFn(DepInst, TLI)) {
     Res = AvailableValue::get(Constant::getNullValue(LI->getType()));
     return true;
   }
 
   if (StoreInst *S = dyn_cast<StoreInst>(DepInst)) {
     // Reject loads and stores that are to the same address but are of
     // different types if we have to. If the stored value is larger or equal to
     // the loaded value, we can reuse it.
-    if (S->getValueOperand()->getType() != LI->getType() &&
-        !canCoerceMustAliasedValueToLoad(S->getValueOperand(),
-                                         LI->getType(), DL))
+    if (!canCoerceMustAliasedValueToLoad(S->getValueOperand(), LI->getType(),
+                                         DL))
       return false;
 
     // Can't forward from non-atomic to atomic without violating memory model.
     if (S->isAtomic() < LI->isAtomic())
       return false;
 
     Res = AvailableValue::get(S->getValueOperand());
     return true;
   }
 
   if (LoadInst *LD = dyn_cast<LoadInst>(DepInst)) {
     // If the types mismatch and we can't handle it, reject reuse of the load.
     // If the stored value is larger or equal to the loaded value, we can reuse
     // it.
-    if (LD->getType() != LI->getType() &&
-        !canCoerceMustAliasedValueToLoad(LD, LI->getType(), DL))
+    if (!canCoerceMustAliasedValueToLoad(LD, LI->getType(), DL))
       return false;
 
     // Can't forward from non-atomic to atomic without violating memory model.
     if (LD->isAtomic() < LI->isAtomic())
       return false;
 
     Res = AvailableValue::getLoad(LD);
     return true;

llvm/trunk/lib/Transforms/Utils/VNCoercion.cpp

 #include "llvm/Transforms/Utils/VNCoercion.h"
 #include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/Analysis/ConstantFolding.h"
 #include "llvm/Analysis/MemoryDependenceAnalysis.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/Support/Debug.h"
 
 #define DEBUG_TYPE "vncoerce"
 namespace llvm {
 namespace VNCoercion {
 
 /// Return true if coerceAvailableValueToLoadType will succeed.
 bool canCoerceMustAliasedValueToLoad(Value *StoredVal, Type *LoadTy,
                                      const DataLayout &DL) {
+  Type *StoredTy = StoredVal->getType();
+  if (StoredTy == LoadTy)
+    return true;
+
   // If the loaded or stored value is an first class array or struct, don't try
   // to transform them.  We need to be able to bitcast to integer.
-  if (LoadTy->isStructTy() || LoadTy->isArrayTy() ||
-      StoredVal->getType()->isStructTy() || StoredVal->getType()->isArrayTy())
+  if (LoadTy->isStructTy() || LoadTy->isArrayTy() || StoredTy->isStructTy() ||
+      StoredTy->isArrayTy())
     return false;
 
-  uint64_t StoreSize = DL.getTypeSizeInBits(StoredVal->getType());
+  uint64_t StoreSize = DL.getTypeSizeInBits(StoredTy);
 
   // The store size must be byte-aligned to support future type casts.
   if (llvm::alignTo(StoreSize, 8) != StoreSize)
     return false;
 
   // The store has to be at least as big as the load.
   if (StoreSize < DL.getTypeSizeInBits(LoadTy))
     return false;
int analyzeLoadFromClobberingMemInst(Type *LoadTy, Value *LoadPtr,
   // constant memory.
   MemTransferInst *MTI = cast<MemTransferInst>(MI);
 
   Constant *Src = dyn_cast<Constant>(MTI->getSource());
   if (!Src)
     return -1;
 
   GlobalVariable *GV = dyn_cast<GlobalVariable>(GetUnderlyingObject(Src, DL));
-  if (!GV || !GV->isConstant())
+  if (!GV || !GV->isConstant() || !GV->hasDefinitiveInitializer())
     return -1;
 
   // See if the access is within the bounds of the transfer.
   int Offset = analyzeLoadFromClobberingWrite(LoadTy, LoadPtr, MI->getDest(),
                                               MemSizeInBits, DL);
   if (Offset == -1)
     return Offset;