[PATCH] Commoning of target specific load/store intrinsics in Early CSE

[Pete Cooper]

Thanks for fixing all my prior comments.

The patch LGTM with just the few additional comments I made.  I'm no expert on this part of the compiler though, so it would be best if Hal or someone else can give the final LGTM.

Thanks,
Pete


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:39
@@ -37,1 +38,3 @@
 
+/// MemIntrinsicInfo - This structure contains information about a load/store
+/// intrinsic defined by the target.
----------------
Please use \brief here instead of 'MemIntrinsicInfo -'

================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:43
@@ +42,3 @@
+  MemIntrinsicInfo()
+      : ReadMem(false), WriteMem(false), MatchingId(0), PtrVal(nullptr) {}
+  bool ReadMem;
----------------
You need to initialize Vol and NumMemRefs here

================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:463
@@ +462,3 @@
+
+  /// \returns An instruction defining the result of a memory intrinsic.  New
+  /// instructions may be created to extract the result from an intrinsic memory
----------------
I think this comment should say it returns a value not an instruction.  I think its possible for your code to return a Value here if the st2 intrinsic was storing constants for example.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:408
@@ +407,3 @@
+        Vol = !LI->isSimple();
+        Ptr = LI->getOperand(0);
+      } else if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
----------------
I think this can also be getPointerOperand() as you've done on the store

http://reviews.llvm.org/D7121

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