[EarlyCSE] Small refactoring changes, NFC

[kparzysz]

Summary

1. Store intrinsic ID in ParseMemoryInst instead of a boolean flag IsTargetMemInst. This will make it easier to add support for target-independent intrinsics.
2. Extract the complex multiline conditions from EarlyCSE::processNode into a new function getMatchingValue.

[efriedma]

Mostly looks fine; couple minor suggestions.

llvm/lib/Transforms/Scalar/EarlyCSE.cpp

983

Is InValFirst the same thing as !MemInst.isLoad()? Is there some useful semantic difference I'm missing?

1294

Unnecessary InVal.DefInst &&?

[kparzysz]

llvm/lib/Transforms/Scalar/EarlyCSE.cpp

1294

The lookup can return an "empty" LoadValue, one that has DefInst set to nullptr. In that case, getMatchingValue will also return nullptr, so we need this check.

[kparzysz]

Removed the bool argument to getMatchingValue. Added comments explaining how the return value is used.

[efriedma]

LGTM

[nikic]

FWIW there were code size changes with this patch, so it wasn't NFC.

[fhahn]

I think it looks like this patch is causing EarlyCSE to crash with expensive checks enabled: http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/17662/consoleFull#-261651283a1ca8a51-895e-46c6-af87-ce24fa4cd561. This commit is the only CSE/MemorySSA related change in the build that started crashing.

It would be great if you could take a look or revert if it takes longer to fix.

[kparzysz]

In D87691#2289581, @fhahn wrote:

It would be great if you could take a look or revert if it takes longer to fix.

Sure, let me check.

[kparzysz]

In D87691#2289933, @kparzysz wrote:

In D87691#2289581, @fhahn wrote:

It would be great if you could take a look or revert if it takes longer to fix.

Sure, let me check.

Fix is coming soon.

[kparzysz]

Fixed in https://reviews.llvm.org/rGe976fb1e54f3.

[fhahn]

In D87691#2290606, @kparzysz wrote:

Fixed in https://reviews.llvm.org/rGe976fb1e54f3.

Thanks, looks like green dragon is happy now with expensive checks http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/17679/

llvm/lib/Transforms/Scalar/EarlyCSE.cpp

private:
   };
 
   /// Wrapper class to handle memory instructions, including loads,
   /// stores and intrinsic loads and stores defined by the target.
   class ParseMemoryInst {
   public:
     ParseMemoryInst(Instruction *Inst, const TargetTransformInfo &TTI)
       : Inst(Inst) {
-      if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(Inst))
+      if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(Inst)) {
         if (TTI.getTgtMemIntrinsic(II, Info))
-          IsTargetMemInst = true;
+          IntrID = II->getIntrinsicID();
       }
+    }
+
+    Instruction *get() { return Inst; }
+    const Instruction *get() const { return Inst; }
 
     bool isLoad() const {
-      if (IsTargetMemInst) return Info.ReadMem;
+      if (IntrID != 0)
+        return Info.ReadMem;
       return isa<LoadInst>(Inst);
     }
 
     bool isStore() const {
-      if (IsTargetMemInst) return Info.WriteMem;
+      if (IntrID != 0)
+        return Info.WriteMem;
       return isa<StoreInst>(Inst);
     }
 
     bool isAtomic() const {
-      if (IsTargetMemInst)
+      if (IntrID != 0)
         return Info.Ordering != AtomicOrdering::NotAtomic;
       return Inst->isAtomic();
     }
 
     bool isUnordered() const {
-      if (IsTargetMemInst)
+      if (IntrID != 0)
         return Info.isUnordered();
 
       if (LoadInst *LI = dyn_cast<LoadInst>(Inst)) {
         return LI->isUnordered();
       } else if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
         return SI->isUnordered();
       }
       // Conservative answer
       return !Inst->isAtomic();
     }
 
     bool isVolatile() const {
-      if (IsTargetMemInst)
+      if (IntrID != 0)
         return Info.IsVolatile;
 
       if (LoadInst *LI = dyn_cast<LoadInst>(Inst)) {
         return LI->isVolatile();
       } else if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
         return SI->isVolatile();
       }
       // Conservative answer
public:
 
     bool isValid() const { return getPointerOperand() != nullptr; }
 
     // For regular (non-intrinsic) loads/stores, this is set to -1. For
     // intrinsic loads/stores, the id is retrieved from the corresponding
     // field in the MemIntrinsicInfo structure.  That field contains
     // non-negative values only.
     int getMatchingId() const {
-      if (IsTargetMemInst) return Info.MatchingId;
+      if (IntrID != 0)
+        return Info.MatchingId;
       return -1;
     }
 
     Value *getPointerOperand() const {
-      if (IsTargetMemInst) return Info.PtrVal;
+      if (IntrID != 0)
+        return Info.PtrVal;
       return getLoadStorePointerOperand(Inst);
     }
 
     bool mayReadFromMemory() const {
-      if (IsTargetMemInst) return Info.ReadMem;
+      if (IntrID != 0)
+        return Info.ReadMem;
       return Inst->mayReadFromMemory();
     }
 
     bool mayWriteToMemory() const {
-      if (IsTargetMemInst) return Info.WriteMem;
+      if (IntrID != 0)
+        return Info.WriteMem;
       return Inst->mayWriteToMemory();
     }
 
   private:
-    bool IsTargetMemInst = false;
+    Intrinsic::ID IntrID = 0;
     MemIntrinsicInfo Info;
     Instruction *Inst;
   };
 
   bool processNode(DomTreeNode *Node);
 
   bool handleBranchCondition(Instruction *CondInst, const BranchInst *BI,
                              const BasicBlock *BB, const BasicBlock *Pred);
 
+  Value *getMatchingValue(LoadValue &InVal, ParseMemoryInst &MemInst,
+                          unsigned CurrentGeneration, bool InValFirst);
+
   Value *getOrCreateResult(Value *Inst, Type *ExpectedType) const {
     if (auto *LI = dyn_cast<LoadInst>(Inst))
       return LI;
     if (auto *SI = dyn_cast<StoreInst>(Inst))
       return SI->getValueOperand();
     assert(isa<IntrinsicInst>(Inst) && "Instruction not supported");
     return TTI.getOrCreateResultFromMemIntrinsic(cast<IntrinsicInst>(Inst),
                                                  ExpectedType);
if (MatchBinOp(Curr, PropagateOpcode))
         if (Instruction *OPI = dyn_cast<Instruction>(Op))
           if (SimpleValue::canHandle(OPI) && Visited.insert(OPI).second)
             WorkList.push_back(OPI);
   }
 
   return MadeChanges;
 }
 
+Value *EarlyCSE::getMatchingValue(LoadValue &InVal, ParseMemoryInst &MemInst,
+                                  unsigned CurrentGeneration, bool InValFirst) {
+  // InValFirst is false for loads, true for stores.
+
+  if (InVal.DefInst == nullptr)
+    return nullptr;
+  if (InVal.MatchingId != MemInst.getMatchingId())
+    return nullptr;
+  // We don't yet handle removing loads with ordering of any kind.
+  if (MemInst.isVolatile() || !MemInst.isUnordered())
+    return nullptr;
+  // We can't replace an atomic load with one which isn't also atomic.
+  if (MemInst.isLoad() && !InVal.IsAtomic && MemInst.isAtomic())
+    return nullptr;
+  Instruction *Earlier = InValFirst ? InVal.DefInst : MemInst.get();
+  Instruction *Later = InValFirst ? MemInst.get() : InVal.DefInst;
+
+  if (!isOperatingOnInvariantMemAt(MemInst.get(), InVal.Generation) &&
+      !isSameMemGeneration(InVal.Generation, CurrentGeneration, InVal.DefInst,
+                           MemInst.get()))
+    return nullptr;
+  return getOrCreateResult(Later, Earlier->getType());
+}
+
 bool EarlyCSE::processNode(DomTreeNode *Node) {
   bool Changed = false;
   BasicBlock *BB = Node->getBlock();
 
   // If this block has a single predecessor, then the predecessor is the parent
   // of the domtree node and all of the live out memory values are still current
   // in this block.  If this block has multiple predecessors, then they could
   // have invalidated the live-out memory values of our parent value.  For now,
if (MemInst.isValid() && MemInst.isLoad()) {
       // If we have an available version of this load, and if it is the right
       // generation or the load is known to be from an invariant location,
       // replace this instruction.
       //
       // If either the dominating load or the current load are invariant, then
       // we can assume the current load loads the same value as the dominating
       // load.
       LoadValue InVal = AvailableLoads.lookup(MemInst.getPointerOperand());
-      if (InVal.DefInst != nullptr &&
-          InVal.MatchingId == MemInst.getMatchingId() &&
-          // We don't yet handle removing loads with ordering of any kind.
-          !MemInst.isVolatile() && MemInst.isUnordered() &&
-          // We can't replace an atomic load with one which isn't also atomic.
-          InVal.IsAtomic >= MemInst.isAtomic() &&
-          (isOperatingOnInvariantMemAt(&Inst, InVal.Generation) ||
-           isSameMemGeneration(InVal.Generation, CurrentGeneration,
-                               InVal.DefInst, &Inst))) {
-        Value *Op = getOrCreateResult(InVal.DefInst, Inst.getType());
-        if (Op != nullptr) {
-          LLVM_DEBUG(dbgs() << "EarlyCSE CSE LOAD: " << Inst
-                            << "  to: " << *InVal.DefInst << '\n');
-          if (!DebugCounter::shouldExecute(CSECounter)) {
-            LLVM_DEBUG(dbgs() << "Skipping due to debug counter\n");
-            continue;
-          }
-          if (!Inst.use_empty())
-            Inst.replaceAllUsesWith(Op);
-          salvageKnowledge(&Inst, &AC);
-          removeMSSA(Inst);
-          Inst.eraseFromParent();
-          Changed = true;
-          ++NumCSELoad;
-          continue;
-        }
-      }
+      if (Value *Op = getMatchingValue(InVal, MemInst, CurrentGeneration,
+                                       false)) {
 
       // Otherwise, remember that we have this instruction.
       AvailableLoads.insert(MemInst.getPointerOperand(),
                             LoadValue(&Inst, CurrentGeneration,
                                       MemInst.getMatchingId(),
                                       MemInst.isAtomic()));
       LastStore = nullptr;
       continue;
for (Instruction &Inst : make_early_inc_range(BB->getInstList())) {
 
     // write back DSE - If we write back the same value we just loaded from
     // the same location and haven't passed any intervening writes or ordering
     // operations, we can remove the write.  The primary benefit is in allowing
     // the available load table to remain valid and value forward past where
     // the store originally was.
     if (MemInst.isValid() && MemInst.isStore()) {
       LoadValue InVal = AvailableLoads.lookup(MemInst.getPointerOperand());
       if (InVal.DefInst &&
-          InVal.DefInst == getOrCreateResult(&Inst, InVal.DefInst->getType()) &&
-          InVal.MatchingId == MemInst.getMatchingId() &&
-          // We don't yet handle removing stores with ordering of any kind.
-          !MemInst.isVolatile() && MemInst.isUnordered() &&
-          (isOperatingOnInvariantMemAt(&Inst, InVal.Generation) ||
-           isSameMemGeneration(InVal.Generation, CurrentGeneration,
-                               InVal.DefInst, &Inst))) {
+          InVal.DefInst == getMatchingValue(InVal, MemInst, CurrentGeneration,
+                                            true)) {
         // It is okay to have a LastStore to a different pointer here if MemorySSA
         // tells us that the load and store are from the same memory generation.
         // In that case, LastStore should keep its present value since we're
         // removing the current store.
         assert((!LastStore ||
                 ParseMemoryInst(LastStore, TTI).getPointerOperand() ==
                     MemInst.getPointerOperand() ||
                 MSSA) &&