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

[Cameron Zwarich]

On Oct 6, 2010, at 4:45 PM, Jakob Stoklund Olesen wrote:

> 
> 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?

I wanted to wait for this to not cause assertions in the register allocator and pass the tests. Is there any particular compile time test that is good to run for this sort of change?

> 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.

This happens with the handling of phis at the end of AggressivePHICoalescing::SplitInterferencesForBasicBlock(). All of the phis whose uses are on edges from the basic block are processed together. If two phis with the same color interfere, then they don't have the same operand for every shared input, and one of them gets isolated with color 0.

If a phi is isolated, then CSSA::InsertCopiesForPHI inserts copies for all of the operands and the destination, which is always safe regardless of critical edges. Of course, you sometimes could have split a critical edge instead and saved a copy. In that case, the two phis that we treated as interfering wouldn't have been handled at the end of the same block and thus would have never been seen as interfering.

I will take a look at those test cases you posted, though.

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

The problem is NP-complete (it's equivalent to the insertion of copies everywhere followed by the aggressive coalescing of those copies). In practice, it's pretty good (at least according to the papers, we'll find out in LLVM when I can run benchmarks). There are two obvious improvements:

1) When there is an interference, SplitInterferencesForBasicBlock() has to make a choice of the variable to isolate. Right now, it just picks the new variable over the old. It could decide which variable to isolate based on the number of uses or depth in the loop tree.

2) You can combine a sort of value numbering with the interference tests, only considering two values interfering if they have different value numbers. I don't know if we have enough interesting copies left over at this point to make this worthwhile, though. And since we don't have physical registers in phis, we could just run a copy propagation pass beforehand.

> - 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 ;-)

Yeah, probably. ;-) This LiveInterval updating is tricky business.

> 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.

removeRange() will split the LiveRange in two if there is a later use. Is this the correct behavior here?

> - 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.

I only trim the LiveRange of the operand if it ends at the edge. If it ends elsewhere, I don't trim it. It seems from your comment above that this is still wrong.

> - 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).

The PHI operand's LiveRange ends at the edge, not the PHI use index.

>> 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.

Thanks. I'll send out a separate patch for this refactoring.

>> 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.

Is there an efficient way to iterate over all LiveIntervals to update this? I guess I could insert a pass between LiveVariables and LiveIntervals that does this using the existing code in PHIElimination, since it looks like SplitCriticalEdge updates LiveVariables correctly. This pass could then be shared between PHIElimination and StrongPHIElimination.

Cameron