[PATCH] D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable.

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

I'll admit to not staring at the cost model too hard (I rarely have useful input on that kind of target specific thing), but it looked at a glance like you trying to calculate  which might constant fold as well.
If not, or it's not part of the goal, awesome.

i'm happy to whiteboard the safety part with you.
What you are doing would be safe, but maybe not cost effective, if you check and move the entire operand chain until you hit a phi in the same block. It looks like you might be, but it's not entirely clear you are doing that.
The logic in speculatephis and visiting deps/users is hard to follow for me to ensure it comes up with the right list.

The core point, however, to  take the very simple example above:

  a = phi(0, b)
  c = add a, 2
  d = mul c, a

It would not be safe to speculate mul c,a without copying + translating add a, 2 as well.

A valid translation to each predecessor for mul is not:
mul c, 0
and 
mul c, b

it's
cop1 = add 0, 2
mul cop1, 0

and

cop2 = add a, b
mul cop2, b

So you must copy and translate c to translate d.
Also, if you do the speculation in postorder, you are going to presumably reevaluate the same ops again and again in smaller chains (first you do d above, then you do c).

It's unclear to me that you are really doing it in postorder, and not preorder :)
If you are, I imagine it would be better to go the other way around, in preorder, because you could save redundant checks and cut off earlier.
This assumes, however, there is no reason to speculate d above if you choose not to speculate c.


https://reviews.llvm.org/D37467