[EarlyCSE] Fix handling of target memory intrinsics for CSE'ing loads.

[aadg]

Summary

Some target intrinsics can access multiple elements, using the pointer as a
base address (e.g. AArch64 ld4). When trying to CSE such instructions,
it must be checked the available value comes from a compatible instruction
because the pointer is not enough to discriminate whether the value is
correct.

[mcrosier]

Hi Arnaud,
Am I correct in assuming this is a regression caused by r227149, which is a patch I committed on behalf of Sanjin?

Chad

[aadg]

Correct.

[mcrosier]

I've set Sanjin as a reviewer and pinging him internally. Hopefully, he can take a look shortly. Thanks for the patch, Arnaud.

[ssijaric]

Hi Arnaud,

Thank you for the fix. This was an oversight on my part - sorry about that.

Once committed, I am going to rework http://reviews.llvm.org/D8313 to take this fix into account.

LGTM.

Thanks,
Sanjin

lib/Transforms/Scalar/EarlyCSE.cpp

public:
   /// \brief A scoped hash table of the current values of loads.
   ///
   /// This allows us to get efficient access to dominating loads when we have
   /// a fully redundant load.  In addition to the most recent load, we keep
   /// track of a generation count of the read, which is compared against the
   /// current generation count.  The current generation count is incremented
   /// after every possibly writing memory operation, which ensures that we only
   /// CSE loads with other loads that have no intervening store.
-  typedef RecyclingAllocator<
-      BumpPtrAllocator,
-      ScopedHashTableVal<Value *, std::pair<Value *, unsigned>>>
+  struct LoadValue {
+    Value *data;
+    unsigned generation;
+    int matchingId;
+    LoadValue() : data(nullptr), generation(0), matchingId(-1) {}
+    LoadValue(Value *data, unsigned generation, unsigned matchingId)
+        : data(data), generation(generation), matchingId(matchingId) {}
+  };
+  typedef RecyclingAllocator<BumpPtrAllocator,
+                             ScopedHashTableVal<Value *, LoadValue>>
       LoadMapAllocator;
-  typedef ScopedHashTable<Value *, std::pair<Value *, unsigned>,
-                          DenseMapInfo<Value *>, LoadMapAllocator> LoadHTType;
+  typedef ScopedHashTable<Value *, LoadValue, DenseMapInfo<Value *>,
+                          LoadMapAllocator> LoadHTType;
   LoadHTType AvailableLoads;
 
   /// \brief A scoped hash table of the current values of read-only call
   /// values.
   ///
   /// It uses the same generation count as loads.
   typedef ScopedHashTable<CallValue, std::pair<Value *, unsigned>> CallHTType;
   CallHTType AvailableCalls;
if (MemInst.isValid() && MemInst.isLoad()) {
         // Don't CSE across synchronization boundaries.
         if (Inst->mayWriteToMemory())
           ++CurrentGeneration;
         continue;
       }
 
       // If we have an available version of this load, and if it is the right
       // generation, replace this instruction.
-      std::pair<Value *, unsigned> InVal =
-          AvailableLoads.lookup(MemInst.getPtr());
-      if (InVal.first != nullptr && InVal.second == CurrentGeneration) {
-        Value *Op = getOrCreateResult(InVal.first, Inst->getType());
+      LoadValue InVal = AvailableLoads.lookup(MemInst.getPtr());
+      if (InVal.data != nullptr && InVal.generation == CurrentGeneration &&
+          InVal.matchingId == MemInst.getMatchingId()) {
+        Value *Op = getOrCreateResult(InVal.data, Inst->getType());
         if (Op != nullptr) {
           DEBUG(dbgs() << "EarlyCSE CSE LOAD: " << *Inst
-                       << "  to: " << *InVal.first << '\n');
+                       << "  to: " << *InVal.data << '\n');
           if (!Inst->use_empty())
             Inst->replaceAllUsesWith(Op);
           Inst->eraseFromParent();
           Changed = true;
           ++NumCSELoad;
           continue;
         }
       }
 
       // Otherwise, remember that we have this instruction.
-      AvailableLoads.insert(MemInst.getPtr(), std::pair<Value *, unsigned>(
-                                                  Inst, CurrentGeneration));
+      AvailableLoads.insert(
+          MemInst.getPtr(),
+          LoadValue(Inst, CurrentGeneration, MemInst.getMatchingId()));
       LastStore = nullptr;
       continue;
     }
 
     // If this instruction may read from memory, forget LastStore.
     // Load/store intrinsics will indicate both a read and a write to
     // memory.  The target may override this (e.g. so that a store intrinsic
     // does not read  from memory, and thus will be treated the same as a
if (Inst->mayWriteToMemory()) {
           // fallthrough - we can exploit information about this store
         }
 
         // Okay, we just invalidated anything we knew about loaded values.  Try
         // to salvage *something* by remembering that the stored value is a live
         // version of the pointer.  It is safe to forward from volatile stores
         // to non-volatile loads, so we don't have to check for volatility of
         // the store.
-        AvailableLoads.insert(MemInst.getPtr(), std::pair<Value *, unsigned>(
-                                                    Inst, CurrentGeneration));
+        AvailableLoads.insert(
+            MemInst.getPtr(),
+            LoadValue(Inst, CurrentGeneration, MemInst.getMatchingId()));
 
         // Remember that this was the last store we saw for DSE.
         if (!MemInst.isVolatile())
           LastStore = Inst;
       }
     }
   }
 

test/Transforms/EarlyCSE/AArch64/ldstN.ll

+; RUN: opt -S -early-cse < %s | FileCheck %s
+target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64--linux-gnu"
+
+declare { <4 x i16>, <4 x i16>, <4 x i16>, <4 x i16> } @llvm.aarch64.neon.ld4.v4i16.p0v4i16(<4 x i16>*)
+
+; Although the store and the ld4 are using the same pointer, the
+; data can not be reused because ld4 accesses multiple elements.
+define { <4 x i16>, <4 x i16>, <4 x i16>, <4 x i16> } @foo() {
+entry:
+  store <4 x i16> undef, <4 x i16>* undef, align 8
+  %0 = call { <4 x i16>, <4 x i16>, <4 x i16>, <4 x i16> } @llvm.aarch64.neon.ld4.v4i16.p0v4i16(<4 x i16>* undef)
+  ret { <4 x i16>, <4 x i16>, <4 x i16>, <4 x i16> } %0
+; CHECK-LABEL: @foo(
+; CHECK: store
+; CHECK-NEXT: call
+; CHECK-NEXT: ret
+}