[PATCH] D17625: Do not select EhPad BB in MachineBlockPlacement when there is regular BB to schedule

[Amaury SECHET via llvm-commits]

deadalnix added inline comments.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:563
@@ -540,1 +562,3 @@
+    // from least probable landingpads to more probable ones.
+    if (BestBlock && (IsEHPad ^ (BestFreq >= CandidateFreq)))
       continue;
----------------
davidxl wrote:
> Since there is no fundamental difference between 
> entry, hot, then, cold, hlp, lpret, clp vs
> entry, hot, then, cold, clp, hlp, lpret
> 
> I think change at line 563 should be removed -- it is a distraction to the main code IMO.  Some of the test changes can also be avoided.
It can be done if you feel strongly about it. But please consider the following. Quite often, ehpad code flow into each others. See pseudocode :

    invoke @foo to %then unwind label %outerlp
  then:
    invoke @foo to %exit unwind label %innerlp
  exit:
    ret void
  innerlp:
    landingpad { i8*, i32 } cleanup
    br %innercleanup
  innercleanup:
    ; Do inner cleanup
    br %outercleanup
  outerlp:
    landingpad { i8*, i32 } cleanup
    br %outercleanup
  outercleanup:
    ; Do outer cleanup
    resume

Note that this is typical RAII kind of code and/or nested try, or any combiantion of these. Long story short, this kind of pattern are very common.

Now, if you compute the probabilities, outerlp is consider more probable than innerlp, as the preceding invoke is itself more probable. That means that unwinding code ends up being scheduled completely backward every time. Here you'd get :

  outerlp, outercleanup, innerlp, innercleanup // which branch to outercleanup

This is the most simple case, but it if very common to have more level of nesting than that in the unwind path and it gets worse. You end with a "corkscrew" like basic block scheduling which I don't think is good.


http://reviews.llvm.org/D17625