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

[Sanjin Sijaric via llvm-commits]

ssijaric added a comment.

Sorry for taking so long to get back  to this.  The updated patch is revised to address previous comments.  Also moved LoadStoreSite.h to include/llvm/Analysis, since it makes use of TargetTransformInfo.


================
Comment at: include/llvm/IR/LoadStoreSite.h:41
@@ +40,3 @@
+  void setInstruction(ValTy *V) {
+    if (NativeInstr *Inst = dyn_cast<NativeInstr>(V)) {
+      I.setPointer(Inst);
----------------
reames wrote:
> 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.)
Addressed in the updated patch - the state is always updated.

================
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);
----------------
reames wrote:
> I would prefer you not make the variable const rather than need a const_cast here.  
Done in the updated patch.

================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:539
@@ +538,3 @@
+}
+
+bool AArch64TTIImpl::mayTargetIntrinsicReadFromMemory(
----------------
I changed this a little, so that mayTargetIntrinsicReadFromMemory and mayTargetIntrinsicWriteToMemory apply to all intrinsics that are supported by the target (if they choose to do so). 

The default implementation in TargetTransformInfoImplBase is:

bool mayTargetIntrinsicReadFromMemory(const IntrinsicInst *II) {
   return !cast<CallInst>(II)->doesNotAccessMemory();
}

I also introduced bool mayReadFromMemory(const Instruction *I) and bool mayWriteToMemory(const Instruction *I) to TTI that may invoke mayTargetIntrinsicReadFromMemory and mayTargetIntrinsicWriteToMemory, respectively.  This lets us do away with the kludgy check in EarlyCSE.


================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:469
@@ -526,3 +468,3 @@
       // Ignore volatile loads.
-      if (MemInst.isVolatile()) {
+      if (!LS.isSimple()) {
         LastStore = nullptr;
----------------
Updated in the new patch.

================
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());
----------------
Will be done in the updated patch - the bug will be addressed separately.  The bug can only affect non-AArch64 users, of which there are none as far I can see.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:510
@@ -567,1 +509,3 @@
+    StoreSite SS(Inst, TTI);
+    if (Inst->mayReadFromMemory() && !(SS && !SS.mayReadFromMemory()))
       LastStore = nullptr;
----------------
Thanks for clarifying.  Reworked in the updated patch - the checks are now done using TTI.mayReadFromMemory(Inst).


http://reviews.llvm.org/D8313