[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 feel unclear on what you see the safety conditions of this transform are.  I believe yours are off a little :)

The actual "can you perform the transform" safety conditions are as far as i know as follows:

  for each pred of a phi:
     for each operand, translate it through a phi block to the pred:
        if it was translated, it's safe
        if it was not translated:
          depth_first_search of def chain of operand
             if you hit a constant - safe
             if you hit something that properly dominates the phi block - safe
             if you hit a phi node in the same block - unsafe
            // If it's something in the phi block but has no phis, and is safe, you could duplicate it if is safe to speculate. Otherwise it is unsafe

The reason is there are only three cases:

1. Either the operand chain is somehow defined by the phi in the same block - this is unsafe. By definition, if you had speculated this already, it would have been replaced by a phi that could be translated through, and you would have speculated the rest of the operand chain into phis as well.  IE the fact that your current expression only indirectly depends on a phi means you already made a decision not to speculate the operand chain, either for safety or cost reasons.

(Unless your cost model is very strange, i don't see why you are re-evaluating this on each go-around)

2. Or it is defined but not phi-dependent in the block - these can be copied as a chain if you wanted.

3. or it must dominate the block, by definition

If it didn't dominate the block, and didn't flow through the phi, this would be a path around the dominator to the block, which would mean it wasn't really a dominator.

Example of 1:

  for.body.i:
    %tmp1 = phi i1 [ true, %entry ], [ false, %cond.end.i ]
    %f.08.i = phi i32 [ 0, %entry ], [ %inc.i, %cond.end.i ]
    %mul.i = select i1 %cmp1.i, i32 %f.08.i, i32 0
    br i1 %tmp1, label %cond.end.i, label %cond.true.i
  
  cond.true.i:
    ;; Ensure we don't replace this divide with a phi of ops that merges the wrong loop iteration value
    %div.i = udiv i32 %mul.i, %f.08.i
    br label %cond.end.i
  cond.end.i:
       ....
    %inc.i = add nuw nsw i32 %f.08.i, 1

f.08.i is a safe operand. it translates directly through the phi
mul.i is not safe[1].  It depends on a phi recursively. As mentioned, if you had chosen to speculate the operands, it would now be a direct use of a phi (not an indirect one), because you would have converted the op of phis for the operand into a phi of ops.  Thus, had it been converted, you would have translated it above, and it would have been safe.

IE

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

the only way d is safe is if you had chosen to speculate c as well, because then you would have

  pred2:
  cop = add a, b
  
  a = phi(0, b)
  cphi = phi(2, cop)
  d = mul cphi, 30

and now it's a direct use :)

But you didn't choose to speculate c, so either c is itself unsafe for some reason, or it wasn't cost effective. I can't see how you could sanely decide it is safe or cost effective to speculate d , which also requires speculating c.

Note:
This is essentially a duplicate of OpIsSafeForPhiOfOps in NewGVN.cpp.  I cache both safety and non-safety (but didn't bother to make it non-recursive).  You only cache safety, which means you are N^2 if everything is unsafe. the subgraphs don't get safer over time:)

[1]
An interesting problem here:
If you translate udiv.i through  entry predecessor into udiv i32 %mul.i, 0, it will simplify to 0, which is even right.
if you then translate udiv i32 %mul.i, %f.08.i into udiv i32 %mul.i, %inc.i, you may even think this is safe.  The simplifier will simplify it to zero as well (inc.i == f.08.i + 1, mul = select f.08.i, 0. Because it can prove the second op to udiv is greater than the first, the answer must be zero). It's constant, awesome!

You will get phi(0, 0), which is not right.

So you have to be careful.  You can't call the simplifier on the intermediate expressions.
The real translation would be
foo = select %cmp.i %inc.i, i32 0
udiv i32 %foo, %inc.i

Which is 0 | 1, not 0



================
Comment at: include/llvm/Transforms/Scalar/SpeculateAroundPHIs.h:91
+///   ```
+struct SpeculateAroundPHIsPass : PassInfoMixin<SpeculateAroundPHIsPass> {
+  /// \brief Run the pass over the function.
----------------
All of these are not speculation, they are transformations from op of phis, to phi of ops :)
(NewGVN performs this transform when it requires no code insertion)

In that respect, it's a code hoisting transform.


https://reviews.llvm.org/D37467