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

[hfinkel@anl.gov via llvm-commits]

hfinkel added a comment.

I agree with Philip's comments (except, perhaps, for the one on which I've commented below).


================
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 {
----------------
reames wrote:
> 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.  
Philip, why do you feel this is awkward? It seems that if the point of the abstraction is to hide the differentiation between regular load/stores and related target intrinsics. As such, it seems that this is an important part of the abstraction.

================
Comment at: include/llvm/Analysis/LoadStoreSite.h:125
@@ +124,3 @@
+  /// from, or writing to the same memory location.
+  bool MatchInstructionAndLocation(const LoadStoreSiteBase &Other) const {
+    assert(I.getPointer() && "Instruction not set");
----------------
MatchInstructionAndLocation -> matchInstructionAndLocation


http://reviews.llvm.org/D8313