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

[Philip Reames via llvm-commits]

reames added a comment.

One final round of comments.  This looks really really close to ready to check in.  Please make the requested changes (and nothing else) and I'll give it a LGTM once the patch is updated.  Any further enhancements should go in separate follow on patches.


================
Comment at: include/llvm/Analysis/LoadStoreSite.h:47
@@ +46,3 @@
+      return;
+    if (NativeInstr *Inst = dyn_cast<NativeInstr>(V)) {
+      I.setPointer(Inst);
----------------
auto

================
Comment at: include/llvm/Analysis/LoadStoreSite.h:99
@@ +98,3 @@
+  /// type of instruction.  Given a load instruction, a compatible store
+  /// instruction is the one that is a mirror of the load instruction.
+  bool isCompatible(const Value *Other) const {
----------------
This API feels really awkward.  For now, I'll ask that you pull this function out of the helper class and make it a static function in EarlyCSE.  That at least localizes the awkwardness to the place it's used.  

================
Comment at: include/llvm/Analysis/LoadStoreSite.h:122
@@ +121,3 @@
+
+  /// MatchInstructionAndLocation - true if Other is a compatible load or store
+  /// instruction, reading
----------------
Same as previous.

================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:501
@@ +500,3 @@
+  /// purposes.
+  bool areInlineCompatible(const Function *Caller,
+                           const Function *Callee) const;
----------------
It looks like you may have merged two patches by accident here?

================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:504
@@ +503,3 @@
+
+  /// \returns True if the target intrinsic behaves like a load instruction.
+  /// Must have one memory operand.
----------------
Important detail that needs to be in this comment: not all load like target instructions will return true.  This is correct because the optimizer must treat intrinsics calls conservatively.  i.e. this is an optimization hook the target can hook into, not a correctness hook.  

================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:547
@@ +546,3 @@
+  ///
+  /// This allows the target to override the default mayWriteToMemory
+  /// query for those intrinsics not marked with readnone or readonly
----------------
Why not just mark the intrinsics with readonly or readnone?  Unless you have a very good reason, please do that.  

================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:557
@@ +556,3 @@
+  /// result by invoking mayTargetIntrinsicReadFromMemory.
+  bool mayReadFromMemory(const Instruction *I) const;
+
----------------
This is the wrong place for this API.  It should probably be on LoadSiteBase.  It doesn't need to be part of TTI.  

================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:564
@@ -530,1 +563,3 @@
+  /// result by invoking mayTargetIntrinsicWriteToMemory.
+  bool mayWriteToMemory(const Instruction *I) const;
 
----------------
Same as above.

================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:334
@@ +333,3 @@
+  bool isTargetIntrinsicAtomic(const IntrinsicInst *II) {
+    assert(false &&
+           "Target does not support querying of target memory intrinsics.");
----------------
A slightly clearer way to phrase these assertions would be that the intrinsic passes either the LikeLoad or LikeStore predicate.  This allows a target to override those two and not have to update the others to get a conservatively correct answer.  

================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:485
@@ +484,3 @@
+// AArch64 load/store intrinsics are not atomic.
+bool AArch64TTIImpl::isTargetIntrinsicAtomic(const IntrinsicInst *Inst) {
+  return false;
----------------
You should assert that the intrinsic is in either the load or store list.  Otherwise, this could return an incorrect result for an unknown intrinsic.  

Alternatively, it could be:
if intrinsic in [known list] return false;
else return true;

================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:490
@@ +489,3 @@
+// AArch64 load/store intrinsics are not volatile.
+bool AArch64TTIImpl::isTargetIntrinsicVolatile(const IntrinsicInst *Inst) {
+  return false;
----------------
Same.

================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:504
@@ -470,3 +503,3 @@
     // Create a struct type
-    StructType *ST = dyn_cast<StructType>(ExpectedType);
+    const StructType *ST = dyn_cast<StructType>(ExpectedType);
     if (!ST)
----------------
Submit separately please.

================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:566
@@ +565,3 @@
+  // TODO:  Check for atomicity and volatility.
+  if (isTargetIntrinsicLikeStore(Inst))
+    return false;
----------------
If you rephrase the assertions above, you can just call the existing check functions and remove all these comments.  Please do so.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:295
@@ -293,3 +294,3 @@
       BumpPtrAllocator,
-      ScopedHashTableVal<Value *, std::pair<Value *, unsigned>>>
+      ScopedHashTableVal<const Value *, std::pair<Value *, unsigned>>>
       LoadMapAllocator;
----------------
Separable change.  Please submit separately.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:528
@@ -585,8 +527,3 @@
     // 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
-    // regular store for commoning purposes).
-    if (Inst->mayReadFromMemory() &&
-        !(MemInst.isValid() && !MemInst.mayReadFromMemory()))
+    if (TTI.mayReadFromMemory(Inst))
       LastStore = nullptr;
----------------
Ah, given where this is used, ignore my comment about putting this on LoadStoreBase.  Making it a static function in EarlyCSE is better for the moment.  Still not on TTI, but LSB isn't a good place either.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:560
@@ -623,1 +559,3 @@
+      StoreSite SS(Inst, TTI);
+      if (SS) {
         // We do a trivial form of DSE if there are two stores to the same
----------------
Use the assignment idiom as above.


http://reviews.llvm.org/D8313