[PATCH] D31772: [PowerPC] Add pass to expand extra memcpy calls

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Target/PowerPC/PPCLowerMemIntrinsics.cpp:117
+// size is a compile time constant or not. Will go away when the old memcpy
+// expansion implementation does.
+static void ppcExpandMemCpyAsLoop(MemCpyInst *MI,
----------------
sfertile wrote:
> hfinkel wrote:
> > I don't understand this comment. It looks like this function is just llvm::expandMemCpyAsLoop whenever TTI.useWideIRMemcpyLoopLowering() returns false. Why not just use that function?
> This is so old it took me a while to figure out what was going on here but I think I got it now. This is here essentially because:
> 
> 1)  It originally took pgo data into account but that is split out into a subsequent patch making this the same as ' llvm::expandMemCpyAsLoop when TTI.useWideIRMemcpyLoopLowering() returns false  as you pointed out.
> 
> 2) I didn't use ' llvm::expandMemCpyAsLoop'  directly because  useWideIRMemcpyLoopLowering is actually off by default for every target,  even PPC(!).  
> 
> The reason 'useWideIRMemcpyLoopLowering ' got added in the first place was because I was hesitant to change the targets I didn't have a way of running functional testing for (amd and nvptx backends), and it was only there to give people an easy way to test it out before flipping the switch to the new implementation. It was supposed too get removed in a subsequent cleanup patch that I never implemented.
> 
> Rather then  changing this to use 'llvm::expandMemCpyAsLoop' I would like to leave it since the follow up patch (D32872) modifies it so it no longer matches, and I will post the cleanup patch that removes the extra TTI hook and the old byte-copy only implementation of memcpy lowering separately since it can go in independent of this.
Okay.


================
Comment at: lib/Target/PowerPC/PPCLowerMemIntrinsics.cpp:33
+// [MemcpyLoopFloor, MemcpyLoopCeil] will be expanded. For unknown sizes we are
+// currently expanding all call sites. The pass is off by default and can be
+// enabled with 'ppc-enable-memcpy-loops=true'.
----------------
Please remove "The pass is off by default and can be...". These kinds of comments should, by definition, be true only temporarily, and the risk of becoming stale is high. Plus, nearly all passes have these kinds of cl::opts, and we don't have these kinds of comments in general. Feel free to add a comment below, near the relevant cl::opt, stating that this option enables or disables the entire transformation.


================
Comment at: lib/Target/PowerPC/PPCLowerMemIntrinsics.cpp:101
+      !CPUCheck(CPUAttr.getValueAsString())) {
+    return false;
+  }
----------------
You should allow this check to be overridden when the user explicitly enables the transformation.  You can do this by checking `EnableMemcpyExpansionPass.getNumOccurrences() > 0 && EnableMemcpyExpansionPass`.


================
Comment at: lib/Target/PowerPC/PPCLowerMemIntrinsics.cpp:117
+// size is a compile time constant or not. Will go away when the old memcpy
+// expansion implementation does.
+static void ppcExpandMemCpyAsLoop(MemCpyInst *MI,
----------------
"Will go away when the old memcpy expansion implementation does." - Please refer here to some specific function (i.e. a particular piece of code), so that we know if this comment becomes stale.


Repository:
  rL LLVM

https://reviews.llvm.org/D31772