[PATCH] Refactor commoning of target specific load/store intrinsics in EarlyCSE

[Philip Reames]

Overall, heading in the right direction.  I'm still not particularly happy about the complexity in EarlyCSE to handle target intrinsics at all, but this is definitely looking better than what's there currently.

Please update with context.  The current diff is too hard to review in full.  Some comments given, but they're really incomplete.


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:471
@@ +470,3 @@
+  /// access.
+  bool isTargetMemoryIntrinsicAtomic(const IntrinsicInst *II) const;
+
----------------
The naming of these functions is inconsistent.  I'm fine with either scheme, but please be consistent.  

================
Comment at: include/llvm/IR/LoadStoreSite.h:1
@@ +1,2 @@
+//===- LoadStoreSite.h - Abstract Load and Store instructions ---*- C++ -*-===//
+//
----------------
I might call this something like MemoryAccessSite.h or the like.  Mild preference, don't bother changing this until other things are settled.  

================
Comment at: include/llvm/IR/LoadStoreSite.h:33
@@ +32,3 @@
+          typename IntrinsicInstTy = IntrinsicInst>
+class LoadStoreSiteBase {
+protected:
----------------
I might call this MemoryAccessBase, but that's a minor preference.  My thought process is that it allows a logic extension for an AtomicRMWSite at some point in the future.

================
Comment at: include/llvm/IR/LoadStoreSite.h:69
@@ +68,3 @@
+    InstrTy *Inst = I.getPointer();
+    if (I.getInt())
+      return cast<NativeInstr>(Inst)->getPointerOperand();
----------------
I'd wrap getInt in a helper function which makes the purpose clearer to the reader.  The following seems cleaner and easier to read:

if (I.isTargetIntrinsic())
  TTI->getTargetMemoryIntrinsicPointerOperand(
 cast<IntrinsicInstTy>(Inst));
return cast<NativeInstr>(Inst)->getPointerOperand();

================
Comment at: include/llvm/IR/LoadStoreSite.h:76
@@ +75,3 @@
+  LoadStoreSiteBase &operator=(InstrTy *Inst) {
+    I.setPointer(nullptr);
+    I.setInt(false);
----------------
You've got this logic duplicated in a few places.  Please common.

================
Comment at: include/llvm/IR/LoadStoreSite.h:99
@@ +98,3 @@
+      return Inst->mayReadFromMemory();
+    return TTI->mayTargetMemoryIntrinsicReadFromMemory(
+        cast<IntrinsicInst>(Inst));
----------------
Having this here is sorta okay, but it really feels like this needs to live somewhere that applies easily to all intrinsics.  We have the readonly/readnone attributes, why not just mark the underling intrinsics appropriately?

================
Comment at: include/llvm/IR/LoadStoreSite.h:110
@@ +109,3 @@
+      if (const LoadInst *LI = dyn_cast<LoadInst>(Other))
+        return getPointerOperand()->getType() ==
+               LI->getPointerOperand()->getType();
----------------
This shouldn't be necessary.  If EarlyCSE doesn't know how to insert bitcasts, it really, really should.  

================
Comment at: include/llvm/IR/LoadStoreSite.h:146
@@ +145,3 @@
+  }
+  static bool isTargetMemoryIntrinsicSupported(const IntrinsicInst *II,
+                                               const TargetTransformInfo *TTI) {
----------------
This should probably be private.

================
Comment at: lib/Analysis/TargetTransformInfo.cpp:269
@@ +268,3 @@
+
+bool TargetTransformInfo::isTargetMemoryIntrinsicAtomic(
+    const IntrinsicInst *II) const {
----------------
What do you mean by "Atomic" here?  AtomicRMW (the instruction?).  Atomic (as in memory ordering?)  The naming is very unclear.

================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:539
@@ +538,3 @@
+    const IntrinsicInst *Inst) {
+  if (isTargetIntrinsicLikeLoad(Inst))
+    return true;
----------------
Surely there are other intrinsics which read from memory?  I really doubt the correctness of this code.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:469
@@ -526,3 +468,3 @@
       // Ignore volatile loads.
-      if (MemInst.isVolatile()) {
+      if (!LS.isSimple()) {
         LastStore = nullptr;
----------------
Volatile != Simple.  Please use appropriate naming to match underling instruction.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:482
@@ -540,1 +481,3 @@
+      if (InVal.first != nullptr && InVal.second == CurrentGeneration &&
+          LS.isCompatible(InVal.first)) {
         Value *Op = getOrCreateResult(InVal.first, Inst->getType());
----------------
Er, is this a separate bugfix?  I don't understand where this is coming from w.r.t. the requested refactor.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:510
@@ -567,1 +509,3 @@
+    StoreSite SS(Inst, TTI);
+    if (Inst->mayReadFromMemory() && !(SS && !SS.mayReadFromMemory()))
       LastStore = nullptr;
----------------
It really feels like the functionality for whether an intrinsic reads from memory should live inside mayReadFromMemory.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:545
@@ -600,5 +544,3 @@
         if (LastStore) {
-          ParseMemoryInst LastStoreMemInst(LastStore, TTI);
-          if (LastStoreMemInst.isMatchingMemLoc(MemInst)) {
-            DEBUG(dbgs() << "EarlyCSE DEAD STORE: " << *LastStore
-                         << "  due to: " << *Inst << '\n');
+          if (LastStore.Match(SS)) {
+            DEBUG(dbgs() << "EarlyCSE DEAD STORE: "
----------------
Given this is testing whether a location matches not whether the instruction matches, I would like to see something which made that explicit.  

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:567
@@ -621,3 +566,3 @@
         // Remember that this was the last store we saw for DSE.
-        if (!MemInst.isVolatile())
+        if (SS.isSimple())
           LastStore = Inst;
----------------
a) naming, b) inverted conditional (bug!)

http://reviews.llvm.org/D8313

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/