[PATCH] D34005: [CGP / PowerPC] avoid multi-block overhead for simple memcmp expansion

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D34005#775445, @courbet wrote:

> LGTM, you might want to wait for other comments as I'm new here :)


Thanks for the quick review! I'll give the other reviewers a little more time to comment in case they see any problems/improvements.



================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:1703
 
-  IRBuilder<> Builder(CI->getContext());
-  BasicBlock *StartBlock = CI->getParent();
-  EndBlock = StartBlock->splitBasicBlock(CI, "endblock");
-  setupEndBlockPHINodes();
+  // A memcmp with zero-comparison with only one block of load and compare does
+  // not need to set up any extra blocks.
----------------
courbet wrote:
> I think the comment should also explain why in this case (only one block) we don't want to abort everything right now and let the SDAG do the lowering (IIUC, something along the lines of "in that case, we still want to do the memcmp expansion here because this code handles vector expansions better").
Sure. I think there will be scalar expansions that are also better handled here if we lift MemCmpNumLoadsPerBlock above '1'. It could have been done in the SDAG, but now that we have this general infrastructure here, I think it's better to keep the memcmp expansion together.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:1729
+  IRBuilder<> Builder(CI->getContext());
   Builder.SetCurrentDebugLocation(CI->getDebugLoc());
 }
----------------
courbet wrote:
> The debug location is never used; this looks like a remnant of some previous code before refactoring. It looks like the intent was to use that builder in member functions.  Now the builder is recreated every time (see e.g. MemCmpExpansion::emitLoadCompareByteBlock, line 1750). Builder should be made a member (maybe in another revision).
This hasn't changed since the initial commit of D28637 / rL304313, but I agree we should clean it up. I'll make that a follow-up step.


https://reviews.llvm.org/D34005