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

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added a comment.

> Do you see any performance difference in any of your benchmarks (i.e. other than Xalan and h264ref)?

I try to test this on AArch64 and share the result.



================
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:
----------------
nemanjai wrote:
> 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).
Based on your summary and function name (dupRetFedByEmptyBlocks), I just expected that you check if BB is empty. However, your implementation doesn't seem to  really check if BB is empty or not. So, I wanted to understand your intention. 

I also doubt if BB really need to be a kind of empty? For me, it seems okay to duplicate return to BB even when BB is not empty as you implement right now. 

I think this need to be done before eliminateMostlyEmptyBlocks(), but BB doesn't need to be empty. 

Please let me know if there is any downside of doing this when BB is non-empty.


================
Comment at: test/CodeGen/MIR/Generic/early-ret-bb.mir:1
+# RUN: llc -run-pass codegenprepare -o - %s | FileCheck %s
+
----------------
nemanjai wrote:
> 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.
Why don't you use opt  : 
  opt < %s -codegenprepare


Repository:
  rL LLVM

https://reviews.llvm.org/D36504