Possible Remat Bug

Project Infrastructure LLVM Dev List Archives
1

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

2

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

3

Jakob Stoklund Olesen <stoklund@2pi.dk> writes:

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

#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. :slight_smile:

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

4

Yes, 3.0 defaults to RAGreedy which uses the new spilling framework. It is ignoring the -spiller=... command line option.

Also note that SplitKit does cheap-as-a-copy rematerialization when splitting live ranges. Both go through LRE.

Linear scan in 3.0 still uses the old stuff, that's why it wasn't deleted sooner.

/jakob

5

Jakob Stoklund Olesen <stoklund@2pi.dk> writes: