[PATCH] D33946: [InlineCost] Find identical loads in single block function

[Haicheng Wu via Phabricator via llvm-commits]

haicheng added a comment.

In https://reviews.llvm.org/D33946#779305, @chandlerc wrote:

> Really cool!
>
> After reading all of this and writing the comments below, I'd switch the term 'CSE' everywhere for 'Elimination'. I know its longer, but I think it is easier for people to read and not get confused about "but what is the subexpression?". This is all really just load elimination.


Thank you very much for looking at this patch.  I agree that this change does not have to depend on SROA.  Please see the inlined reply about enabling this change for multi-block function.



================
Comment at: lib/Analysis/InlineCost.cpp:1469-1470
       SingleBB = false;
+      if (EnableCSELoad)
+        disableCSELoad();
     }
----------------
chandlerc wrote:
> Why?
> 
> One thing the inline cost analysis really tries to do is "straighten" CFGs that will end up folded when inlined, so it would be really good to allow simple CFGs to survive this much like my example above...
> 
> Especially with the initial policy of completely disabling the savings if we see a store *anywhere*.
I observed some regressions when lifting this check.  When two repeated loads are far away from each other, LLVM may not be able to reduce the number of loads.  For example, LoadPre can add one load when it removes one.

I think it is more safe to support arbitrary CFG with the help of DominatorTree.  Only the repeated loads dominated by other identical loads can be simplified.  I think we need DominatorTree anyway if we try to address FIXME #1 you listed above because we need to iterate blocks in  the reverse post order.  So, I am going to add the support of multi-block function as another item in FIXME.

Your example above is still supported as long as SROA requirement is gone because InlineCost can figure out that the callee has single block after inlining.


Repository:
  rL LLVM

https://reviews.llvm.org/D33946