[PATCH] D36504: [CodeGenPrepare][WIP] Convert uncond. branch to return into a return to help with shrink-wrapping

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

In https://reviews.llvm.org/D36504#836904, @junbuml wrote:

> - Overall, I agree with the idea. I didn't see any performance differences in Xalancbmk and h264ref on AArach64 though.  I guess this will not leave a branch instruction in the empty block as it directly return.
> - Should the predecessor branching to an empty return block also be empty? Your implementation doesn't seem to check if the predecessor is empty.
> - Just curious about the -5% in h264ref on PPC?


Do you see any performance difference in any of your benchmarks (i.e. other than Xalan and h264ref)?
I haven't investigated the degradation on h264ref yet, but I do plan to look into it before making further updates to this patch. I figured it'd be nice to hear from other targets as well about whether this has any impact.



================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:653
+  for (BasicBlock &BB : F)
+    MadeChange |= dupRetFedByEmptyBlocks(&BB);
+  return MadeChange;
----------------
junbuml wrote:
> Isn't it possible to do this in the main loop of eliminateMostlyEmptyBlocks()?
I initially had it there but removed it for two reasons (neither of which I feel too strongly about):
1. It just didn't feel like it should go there since I'm doing an inherently different thing
2. That loop skips the entry block and I didn't see a compelling reason to do so for this. Of course, I could just have one invocation before the loop but this looked cleaner.

As I said, I'm not opposed to putting it back.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:2625-2627
+/// Look for opportunities to duplicate a return instruction into a predecessor
+/// that does nothing but branch to the return block. Namely, duplicate a return
+/// statement in both of bb0/bb1 below:
----------------
junbuml wrote:
> It seems that you didn't check if the predecessor is empty? Is this what you intended? 
I initially had a check that the predecessor has a single instruction (i.e. the branch). But as it turns out, the predecessor will actually have a PHI as well in the motivating test case (albeit a PHI with a single entry).

Then I got to thinking about whether there's a compelling reason for the predecessor to have to be empty. And I couldn't think of one.
I suppose we can check if it:
- has a single instruction
- has a single non-PHI instruction
- has a single instruction **or** a single predecessor

Not sure which of those would be the most appropriate (I am leaning toward the middle one).


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:2639-2640
+  BasicBlock *SingleSucc = BB->getSingleSuccessor();
+  DEBUG(dbgs() << "Trying to duplicate the successor's return into: ");
+  DEBUG(BB->dump());
+  // Look at blocks that have a single successor
----------------
junbuml wrote:
> I guess you might reduce DEBUGs in this function. Right?
Yeah. It was nice during development to have all of that. And I figured I'd leave it here while this patch is WIP for anyone that wants to play with the patch. But if this becomes a real patch, I'll reduce the DEBUG messages.


================
Comment at: test/CodeGen/MIR/Generic/early-ret-bb.mir:1
+# RUN: llc -run-pass codegenprepare -o - %s | FileCheck %s
+
----------------
junbuml wrote:
> Why do you use .mir, instead of .ll? 
I do have a .ll test case as well. I wanted to also provide a .mir test case to test the actual transformation in CGP rather than just its effect on the final ASM.


Repository:
  rL LLVM

https://reviews.llvm.org/D36504