llvm dev - Nov 2011 - [LLVMdev] Possible Remat Bug

I'm working on some enhancements to rematerialization that I hope to
contribute.  It's mostly working but I am running into one problem.  It
boils down to having spilled a register used by the remat candidate.

I thought this is what getReMatImplicitUse is supposed to handle but
it looks inconsistent to me.  The comment says this:

/// getReMatImplicitUse - If the remat definition MI has one (for now, we only
/// allow one) virtual register operand, then its uses are implicitly using
/// the register. Returns the virtual register.
unsigned LiveIntervals::getReMatImplicitUse(const LiveInterval &li,
                                            MachineInstr *MI) const {

But down in the code we have this:

    if (TargetRegisterInfo::isPhysicalRegister(Reg) &&
        !allocatableRegs_[Reg])
      continue;
    // FIXME: For now, only remat MI with at most one register operand.
    assert(!RegOp &&
           "Can't rematerialize instruction with multiple register
operand!");
    RegOp = MO.getReg();
#ifndef NDEBUG
    break;
#endif

So if Reg is an allocatable physical register, it gets returned as an
implicit use.  This directly contradicts the comment when talks about
virtual registers exclusively.

So who's right here?

                            -Dave

On Nov 16, 2011, at 9:15 AM, David Greene wrote:
> I'm working on some enhancements to rematerialization that I hope to
> contribute.
What do you have in mind?
>  It's mostly working but I am running into one problem.  It
> boils down to having spilled a register used by the remat candidate.
> 
> I thought this is what getReMatImplicitUse is supposed to handle but
> it looks inconsistent to me.  The comment says this:
> 
> /// getReMatImplicitUse - If the remat definition MI has one (for now, we
only
> /// allow one) virtual register operand, then its uses are implicitly using
> /// the register. Returns the virtual register.
> unsigned LiveIntervals::getReMatImplicitUse(const LiveInterval &li,
>                                            MachineInstr *MI) const {
I just deleted all of the spiller code in LiveIntervalAnalysis.cpp.  The
LiveIntervals::isReMaterializable() entry point is still around, but it is only
getting called from CalcSpillWeights.cpp.  If we can get rid of that use, it
should be deleted as well.
> #ifndef NDEBUG
>    break;
> #endif
Scary...
> So if Reg is an allocatable physical register, it gets returned as an
> implicit use.  This directly contradicts the comment when talks about
> virtual registers exclusively.
> 
> So who's right here?
You want LiveRangeEdit::allUsesAvailableAt() which performs the same check
today.

The important point is that all registers read by the rematerialized instruction
have the same value at the new site. That applies to both physical and virtual
registers.

There is some ambiguity about instructions reading reserved registers.  For
example, we want to allow instructions reading %rip to be moved, even though the
register clearly has a different value at the new location.

Reserved registers without any defs anywhere in the function are treated as
ambient registers that can be read anywhere.  These registers don't get a
LiveInterval, and are skipped here:

    // Reserved registers are OK.
    if (MO.isUndef() || !lis.hasInterval(MO.getReg()))
      continue;

Reserved registers with defs get LiveIntervals, and are value-checked just like
virtual registers.

Also note that unreserved, unallocatable registers exist.  For example the ARM
%cpsr status register.

/jakob

Jakob Stoklund Olesen <stoklund at 2pi.dk> writes:
> On Nov 16, 2011, at 9:15 AM, David Greene wrote:
>
>> I'm working on some enhancements to rematerialization that I hope
to
>> contribute.
>
> What do you have in mind?
Rematting more types of loads.
>> /// getReMatImplicitUse - If the remat definition MI has one (for now,
we only
>> /// allow one) virtual register operand, then its uses are implicitly
using
>> /// the register. Returns the virtual register.
>> unsigned LiveIntervals::getReMatImplicitUse(const LiveInterval &li,
>>                                            MachineInstr *MI) const {
>
> I just deleted all of the spiller code in LiveIntervalAnalysis.cpp.  
It's still there in 3.0.  :)
>> #ifndef NDEBUG
>>    break;
>> #endif
>
> Scary...
Yeah, no kidding!
>> So if Reg is an allocatable physical register, it gets returned as an
>> implicit use.  This directly contradicts the comment when talks about
>> virtual registers exclusively.
>> 
>> So who's right here?
>
> You want LiveRangeEdit::allUsesAvailableAt() which performs the same
> check today.
But not in 3.0, right?
> The important point is that all registers read by the rematerialized
> instruction have the same value at the new site. That applies to both
> physical and virtual registers.
Yes, absolutely.  I actually do this check in my own code before I
realized it was in getReMatImplicitUse.  My check is more general.  :)
> There is some ambiguity about instructions reading reserved registers.
> For example, we want to allow instructions reading %rip to be moved,
> even though the register clearly has a different value at the new
> location.
Yes.
> Reserved registers without any defs anywhere in the function are
> treated as ambient registers that can be read anywhere.  These
> registers don't get a LiveInterval, and are skipped here:
>
>     // Reserved registers are OK.
>     if (MO.isUndef() || !lis.hasInterval(MO.getReg()))
>       continue;
Ok, that translates to getReMatImplicitUse pretty well.
> Reserved registers with defs get LiveIntervals, and are value-checked
> just like virtual registers.
Yes.
> Also note that unreserved, unallocatable registers exist.  For example
> the ARM %cpsr status register.
Yes.

So it seems the comment on getReMatImplicitUse in incorrect in that it
looks at virtual and physical registers.

The current (3.0) limitation of one register use is unfortunate but not
a show-stopper for what I need to do.

Thanks for your help, Jakob!

                            -Dave