LiveIntervals::handlePhysicalRegisterDef, unreachable MBBs

Project Infrastructure LLVM Dev List Archives
1

I've just spend some time looking at the above function, and while I
understood what it does, a comment would have helped a lot.

Maybe, something like:

// Determine the end of the live interval for this register
// For physical register, all internvals are within basic blocks so
// we look for the instruction in this basic block which last uses it.

The first loop might have a comment like:

// If the register is dead at instruction which sets it (i.e. not used later)
// the live interval ends at the next instruction

The second loop might have a comment like:

// If the register is not dead at the defining instruction, it must be used
// and killed by some subsequent instruction. Find that instruction now, which
// always exists

Finally, why I've started all this. I forgot to add machine CFG edge and got
misterious crash. I've applied the attached patch, which checks that all
basic blocks are reachable from function entry. Any comments on:

- whether this check is reasonable?
- what's the right place for the check? LiveVariables.cpp is the simplest I
could find, but does not seem right.

- Volodya

LiveVariables.diff (608 Bytes)

2

I've just spend some time looking at the above function, and while I
understood what it does, a comment would have helped a lot.

Thanks for bringing this to my attention!

Maybe, something like:

// Determine the end of the live interval for this register
// For physical register, all internvals are within basic blocks so
// we look for the instruction in this basic block which last uses it.

The first loop might have a comment like:

// If the register is dead at instruction which sets it (i.e. not used later)
// the live interval ends at the next instruction

Actually it ends at the same instruction. Each instruction has 4 slots.
So instruction 0 spans interval indexes 0-3, instruction 1 4-7 and so
on. In the case above the interval added is:

[defSlot(instr), defSlot(instr)+1)

You may notice that this interval is "empty" in the interval indexes
discrete world (i.e. [13,14)). Indeed it is, and its only purpose is to
naturally express this clobbering of physical registers to the register
allocator. For example given [13,14) for some physical register A and a
live interval [0,27) for some virtual register Z, we cannot allocate A
to Z because of the overlap!

The second loop might have a comment like:

// If the register is not dead at the defining instruction, it must be used
// and killed by some subsequent instruction. Find that instruction now, which
// always exists

I added comments to the handlePhysicalRegisterDef. Let me know if it
makes more sense now.

Finally, why I've started all this. I forgot to add machine CFG edge and got
misterious crash. I've applied the attached patch, which checks that all
basic blocks are reachable from function entry. Any comments on:

- whether this check is reasonable?
- what's the right place for the check? LiveVariables.cpp is the simplest I
could find, but does not seem right.

I am not sure about this change. I will let others comment on it.

3

Sure that makes sense, It's good to check invariants instead of crashing
mysteriously! I've applied a modified version of your patch here:
http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040705/015867.html

-Chris

4

Alkis Evlogimenos wrote:

> Maybe, something like:
>
> // Determine the end of the live interval for this register
> // For physical register, all internvals are within basic blocks so
> // we look for the instruction in this basic block which last uses it.
>
> The first loop might have a comment like:
>
> // If the register is dead at instruction which sets it (i.e. not used
> later) // the live interval ends at the next instruction

Actually it ends at the same instruction. Each instruction has 4 slots.
So instruction 0 spans interval indexes 0-3, instruction 1 4-7 and so
on. In the case above the interval added is:

[defSlot(instr), defSlot(instr)+1)

Ok, I did not know about "slots".

You may notice that this interval is "empty" in the interval indexes
discrete world (i.e. [13,14)). Indeed it is, and its only purpose is to
naturally express this clobbering of physical registers to the register
allocator. For example given [13,14) for some physical register A and a
live interval [0,27) for some virtual register Z, we cannot allocate A
to Z because of the overlap!

Yes, I understand that.

I added comments to the handlePhysicalRegisterDef. Let me know if it
makes more sense now.

Yes, definitely. Thanks!

- Volodya