[PATCH] D67318: [SimplifyCFG] FoldTwoEntryPHINode(): consider *total* speculation cost, not per-BB cost

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D67318#1670962 <https://reviews.llvm.org/D67318#1670962>, @jmolloy wrote:

> I can think of a rationale for the current behaviour. A lopsided if-triangle could indicate it might be better to emit a branch than speculate. Especially as this transform is context-free, input code that contains the same pattern many times can cause an explosion in code size and the transform was likely designed to be conservative.
>
> I don't think there's a right answer for SimplifyCFG. Whatever we do is wrong, but sometimes it is useful. I think your approach here has been great; you have a bunch of performance data showing  improvements


Right.

> and no regressions.

To be noted, //on the benchmarks run//.
I will be **very** surprised if this does not cause any regressions at all.

> I also like your renaming from "Cost" to "Budget". That's annoyed me for a while.

Yeah, that one looked really weird to me too.

> Also note that there exist targets that don't use conditional moves to perform predication (most VLIW targets have a dedicated predicate register file) so that changes the behaviour somewhat.



> LGTM.

@jmolloy thank you for the review.
@efriedma / others - any comments? Not sure if i should wait a bit more or proceed to commit here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67318/new/

https://reviews.llvm.org/D67318