Is a PHI use of another PHI in the same block valid?

Project Infrastructure LLVM Dev List Archives
1

Over in pr26718, we ran across a case where input IR had one PHI within a basic block using the value of another PHI within the same basic block (without a backedge). There has been some disagreement as to whether this is valid IR. I believe it is not.

The verifier currently accepts the following code without error:

define void @f() {
entry:
  br label %next

next:
  %y = phi i32 [ 0, %entry ]
  %x = phi i32 [ %y, %entry ]
  ret void
}

Looking at the code, this may be an oversight - due to a special casing of the dominance relation within a single basic block - but that’s not obviously true. Thus, we need to clarify what the intended semantics are.

The lang ref seems pretty clear about this:
“For the purposes of the SSA form, the use of each incoming value is deemed to occur on the edge from the corresponding predecessor block to the current block (but after any definition of an ‘invoke‘ instruction’s return value on the same edge).”

But David pointed out that various bits of the optimizer appear to have code and test cases covering exactly this case.

So, is this legal IR? Or not?

Philip

2

For people not following the PR, Joseph has pointed out some issues
with what the verifier currently accepts in an excellent writeup on
the bug: https://llvm.org/bugs/show_bug.cgi?id=26718#c9

3

I don't know if the IR spec mentions this case, but it simply makes no sense. PHI nodes indicate values on entry to the block. Their ordering in the IR only exists because instructions are arranged in an ordered list, but the relative ordering of PHIs has no meaning.

-Krzysztof

4

It would make sense if there was a back edge to this block---then the value used in the PHI would be the value on exit from this block.

-Krzysztof

5

If we decide not to be legal, should we change the verify to reject
it? What happens to the test cases that currently test this very
situation (eg. Transforms/LoopVectorize/phi-hang.ll)?

The change as suggested by Philip is:

- Assert(InstsInThisBlock.count(Op) || DT.dominates(Op, U),
+ Assert((!isa<PHINode>(I) && InstsInThisBlock.count(Op)) ||
+ DT.dominates(Op, U),
+ "Instruction does not dominate all uses!", Op, &I);

Michael

6

If we decide not to be legal, should we change the verify to reject
it? What happens to the test cases that currently test this very
situation (eg. Transforms/LoopVectorize/phi-hang.ll)?

The change as suggested by Philip is:

- Assert(InstsInThisBlock.count(Op) || DT.dominates(Op, U),
+ Assert((!isa<PHINode>(I) && InstsInThisBlock.count(Op)) ||
+ DT.dominates(Op, U),
+ "Instruction does not dominate all uses!", Op, &I);

Michael

7

FWIW, I just ran "ninja check" with the proposed change.
Transforms/LoopVectorize/phi-hang.ll is the only one that fails.

Michael