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

[Philip Reames]

A few standalone comments before I go through the entire patch again.


================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:539
@@ +538,3 @@
+    const IntrinsicInst *Inst) {
+  if (isTargetIntrinsicLikeLoad(Inst))
+    return true;
----------------
ssijaric wrote:
> reames wrote:
> > Surely there are other intrinsics which read from memory?  I really doubt the correctness of this code.
> There are, but only a few intrinsics are supported for now (the ones in isTargetIntrinsicLikeLoad(...) and isTargetIntrinsicLikeStore(...)).  The assert makes sure that we are dealing with supported load/store intrinsics.
> 
> Perhaps, a better way would be to just have one query function for load/store intrinsics.   Something like
> 
> MemoryIntrinsicInfo getTargetMemoryIntrinsic(IntrinsicInst *Inst);
> 
> which would return all the needed information inside the MemoryIntrinsicInfo struct?  Then we could do away with separate APIs, which may be confusing.  The overhead would be the inclusion of this struct in LoadStoreSiteBase.
I believe you've changed the name since the last version.  Make sure the comments indicate that this only applies to target load and store intrinsics.  I'm then fine with that.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:469
@@ -526,3 +468,3 @@
       // Ignore volatile loads.
-      if (MemInst.isVolatile()) {
+      if (!LS.isSimple()) {
         LastStore = nullptr;
----------------
ssijaric wrote:
> reames wrote:
> > Volatile != Simple.  Please use appropriate naming to match underling instruction.
> Originally, the check was 
> 
> // Ignore volatile loads.
> if (!LI->isSimple())
> 
> The check "if (MemInst.isVolatile())" does the same check underneath, at least for regular loads and stores.  The name of MemInst.isVolatile() is wrong  - it should have been MemInst.isSimple()).
> 
> The new check
> 
> if (!LS.isSimple()) restores the original check that was in place.
> 
> The comment , "// Ignore volatile loads." should probably be "// Ignore volatile and atomic loads".
Update the comment please.  

================
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());
----------------
ssijaric wrote:
> reames wrote:
> > Er, is this a separate bugfix?  I don't understand where this is coming from w.r.t. the requested refactor.
> Yes, this bug was pointed out by Hal, and could affect Power intrinsics from what I understand.  I thought I'd include it here.  If you prefer, I can do this under a seperate bug fix once the refactor is sorted out.
I would strongly prefer this be done separately.  

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:510
@@ -567,1 +509,3 @@
+    StoreSite SS(Inst, TTI);
+    if (Inst->mayReadFromMemory() && !(SS && !SS.mayReadFromMemory()))
       LastStore = nullptr;
----------------
ssijaric wrote:
> reames wrote:
> > It really feels like the functionality for whether an intrinsic reads from memory should live inside mayReadFromMemory.
> AArch64 store intrinsics are not marked as readnone, and mayReadFromMemory will always return true (unlike for regular non-atomic stores).  So, we wouldn't get a chance to common intrinsic stores.  It doesn't look to be legal to mark store intrinsics as readnone, according to the LLVM Language Reference Manual.
You misunderstood my comment.  I was suggesting that the *function* mayReadFromMemory should check whether the intrinsic is a target specific store and return false.  I'm fine with this being a separate refactor, but this type of special case doesn't belong in a particular transform pass when the answer is the same for anyone who asks.

http://reviews.llvm.org/D8313

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