[llvm-commits] Debug-check in SSAUpdater detecting PHI/CFG missmatches

[Bob Wilson]

On Jul 22, 2011, at 9:41 AM, Ludwig Meier wrote:

> On Jul 22, 2011, 09:03 -0700, Bob Wilson wrote:
>> On Jul 16, 2011, at 2:56 AM, Ludwig Meier wrote:
>> 
>>> This patch adds a debug-check to SSAUpdater, that verifies that the
>>> predecessor list (as derived from the CFG) matches the predecessor list
>>> in the PHI-nodes.
>>> 
>>> For performance reasons the SSAUpdater uses the incoming-edges of a
>>> PHI-Node (if one exists) to determine the predecessors of a basic-block.
>>> When the PHI-nodes incoming edges differ from the CFG, subsequent calls
>>> to SSAUpdater::GetValue...() may crash.
>>> This occurs only when the IR is currently invalid, of course.
>> 
>> I think this should go in as a verifier check.  We don't want to have to debugging checks for well-formed IR scattered throughout LLVM.
> 
> The verifier probably already catches this.
> But you wont get to the verifyer in this case, because using the
> SSAUpdater in invalid IR would cause a segfault earlier. To find the
> reason for the crash, you have to analyze the internals of SSAUpdater
> (to figure out that SSAUpdater uses the PHI-nodes to enumerate a blocks
> predecessors).
> With this check added you would hopefully not have to poke around in
> SSAUpaters internals to find whats wrong.
> 
> As a sidenote: In my case I was well aware, that I had invalid
> PHI-nodes. I am using SSAUpdater in the process of fixing them. Now I'm
> syncing the PHI-nodes to the CFG before using the SSAUpdater.

That's a good point.  Perhaps the SSAUpdater should not take advantage of that trick to find the predecessors?  Otherwise, when you change the CFG, you may have to first manually change the existings PHIs to either remove them or at least have the right blocks listed, and then separately run the SSAUpdater.  That doesn't seem like the right thing.  I don't remember offhand how big the performance hit would be.