llvm dev - Jul 2004 - [LLVMdev] LiveIntervals::handlePhysicalRegisterDef, unreachable MBBs

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







-------------- next part --------------
A non-text attachment was scrubbed...
Name: LiveVariables.diff
Type: text/x-diff
Size: 608 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20040709/e0fcec25/attachment.diff>

On Fri, 2004-07-09 at 05:02, Vladimir Prus wrote:
> 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.

-- 

Alkis

On Fri, 9 Jul 2004, Vladimir Prus wrote:
> 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.
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

-- 
http://llvm.cs.uiuc.edu/
http://www.nondot.org/~sabre/Projects/

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