[llvm-commits] [Review request] New StrongPHIElimination implementation

[Jakob Stoklund Olesen]

On Oct 5, 2010, at 3:23 PM, Cameron Zwarich wrote:

> I've been wanting to implement SSA-based register allocation in LLVM, but since that is a bit of a major task, I wanted to do something smaller to get myself more familiar with the backend. I decided to rewrite the StrongPHIElimination pass and get it closer to working. I split the pass up into 3 stages:

Hi Cameron,

This is very interesting. Do you have any compile time measurements?

Some comments:

- I am not really convinced that CSSA::InsertCopiesForPHI deals with critical edges correctly. How do you ensure non-interference when a block branches to multiple PHIs? This happens for instance when dealing with indirect branches, see http://llvm.org/bugs/show_bug.cgi?id=7362.

- I suspect that aggressive PHI coalescing doesn't have a unique solution. How to you choose the optimal congruence classes?

- You are not dealing with register classes at all, and that is probably fine as long as you are only working with PHI uses and defs. Please make the assumption explicit, though. StrongPHIElimination::RenameRegister is really wrong if the registers have different classes.

> 1) While the pass at least completes on the simple examples I tried (unlike the existing code), it causes SimpleRegisterCoalescing to hit an assertion failure. I am not certain whether this is due to a bug in the way the new code updates LiveIntervals or a bad assumption in SimpleRegisterCoalescing that is satisfied by PHIElimination but not StrongPHIElimination.

You're doing it wrong ;-)

The machine code verifier has recently learned to verify LiveIntervals, but it is still incomplete. If you stick "MF.verify(this)" at the end of your runOnMachineFunction functions, it should tell you about some mistakes, though.

First of all, the interval in a LiveRange instance can span multiple basic blocks, and you almost never have enough information to edit it directly, so don't. Use the addRange and removeRange functions instead. I wish LiveRange could be a private implementation detail in LiveInterval, but unfortunately linear scan uses it.

For queries, try to get by with liveAt() and getVNInfoAt() as much as possible.

So instead of this:

+    LiveRange* DestLR = DestInterval.getLiveRangeContaining(PHIIndex);
+    DestLR->valno->setIsPHIDef(true);

Use this:

VNInfo *VNI = DestInterval.getVNInfoAt(PHIIndex);
assert(VNI);
VNI->setIsPHIDef(true);

- Don't dereference pointers that may be NULL without an assertion.

- FindLiveRangeEndingAtIndex is asking the wrong question, and should be removed. You should be able to use getVNInfoAt() and removeRange() instead.

- In CSSA::InsertCopiesForPHI you are not adding liveness from the MBB start index to the PHI use index for your new CopyReg.

- You are assuming that the PHI operand was only used by the PHI when you are trimming the live range. The register could still be live out from the predecessor if you have a critical edge. This is exactly the case where PHIElimination will try to split the critical edge.

- You need to remove the snippet of live range from the MBB start index to the PHI use index from your SrcReg. (The same snippet you forgot to add to CopyReg).

> 2) I had to copy two functions from PHIElimination, SkipPHIsAndLabels() and FindCopyInsertPoint(). Presumedly, these should go somewhere shared, but where?

They probably belong in MachineBasicBlock, somewhere near getFirstTerminator.

> 3) It handles critical edges, but it still might be better to split them sometimes.

Yes, you should try to split critical edges when PHIElimination does it: When you would be creating extra interference because you inserted a copy at the end of a block, and both the use and the def are live out of the block.

Note that splitting critical edges while preserving LiveIntervals is tricky. Any variable that is live across the edge must be marked as live through the new block.


I haven't really worked with PHI instructions and LiveIntervals being live at the same time before. Setting the phi-def and phi-kill flags on LiveIntervals during CSSA seems wrong to me, though.

In spite of what the LiveInterval.h header says, I think LiveIntervals should treat PHI instructions like any other instruction. There should be no phi-def and phi-kill flags while PHI instructions are present.

A VNInfo with the phi-def flag set should be defined at an MBB start index, not by an instruction. Similarly, a VNInfo with the phi-kill flag set is live out of an MBB with a phi-def successor.

In other words, the phi-def and phi-kill flags should indicate that the VNInfo has defs and uses at MBB edges, not just at instructions.

That means you should be setting these flags in StrongPHIElimination::runOnMachineFunction when you are actually removing the PHI instructions.

/jakob