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

[Philip Reames]

Ok, I'm running out of comments to make.  The EarlyCSE code is much improved here; thank you for taking the time to do this.

I'm not entirely thrilled with how complex the target interface is, but that can be addressed incrementally.  What you proposing appears to be correct and is now clearly documented enough to not be confusing.

If you do last one last update with all the naming and small things addressed, I'll do one more run through for a final LGTM.  I considered giving a conditional LGTM now, but there a few too many issues that were deferred.  (In particular, separation of unrelated bug fixes and naming.)  The next round should be fast though.


================
Comment at: include/llvm/IR/LoadStoreSite.h:41
@@ +40,3 @@
+  void setInstruction(ValTy *V) {
+    if (NativeInstr *Inst = dyn_cast<NativeInstr>(V)) {
+      I.setPointer(Inst);
----------------
Leaving the member state unchanged after a call to setInstruction seems bug prone at best.  Please either assert or clear the underlying data.  (Looking through the calling code, I don't believe this is currently an issue.  This is purely future proofing.)

================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:495
@@ -461,3 +494,3 @@
     }
-    Value *Res = UndefValue::get(ExpectedType);
+    Value *Res = UndefValue::get(const_cast<Type *>(ExpectedType));
     IRBuilder<> Builder(Inst);
----------------
I would prefer you not make the variable const rather than need a const_cast here.

http://reviews.llvm.org/D8313

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