I've been playing around with spillers and found that the SimpleSpiller
fails
badly on a particular code.
The problem arises because SimpleSpiller does the test
VRM.isAssignedReg(virtReg) which is implemented as:
00183 bool isAssignedReg(unsigned virtReg) const {
00184 if (getStackSlot(virtReg) == NO_STACK_SLOT &&
00185 getReMatId(virtReg) == NO_STACK_SLOT)
00186 return true;
00187 // Split register can be assigned a physical register as well as a
00188 // stack slot or remat id.
00189 return (Virt2SplitMap[virtReg] && Virt2PhysMap[virtReg] !=
NO_PHYS_REG);
00190 }
VRM::assignVirt2Phys is implemented as:
00147 void assignVirt2Phys(unsigned virtReg, unsigned physReg) {
00148 assert(TargetRegisterInfo::isVirtualRegister(virtReg) &&
00149 TargetRegisterInfo::isPhysicalRegister(physReg));
00150 assert(Virt2PhysMap[virtReg] == NO_PHYS_REG &&
00151 "attempt to assign physical register to already mapped
"
00152 "virtual register");
00153 Virt2PhysMap[virtReg] = physReg;
00154 }
Note that this doesn't clear the stack slot or remat mappings.
In the particular code I'm looking at, linear scan spills an interval and
one of the intervals created (%reg1631) gets mark as rematerializable by
LiveIntervals:
00984 if (CreatedNewVReg) {
00985 if (DefIsReMat) {
00986 vrm.setVirtIsReMaterialized(NewVReg, ReMatDefMI/*, CanDelete*/);
00987 if (ReMatIds[VNI->id] == VirtRegMap::MAX_STACK_SLOT) {
00988 // Each valnum may have its own remat id.
00989 ReMatIds[VNI->id] = vrm.assignVirtReMatId(NewVReg);
Linear scan dutifully assigns %reg1631 to AL. Then SimpleSpiller runs and
does this:
00261 unsigned VirtReg = MO.getReg();
00262 unsigned PhysReg = VRM.getPhys(VirtReg);
00263 if (!VRM.isAssignedReg(VirtReg)) {
00264 int StackSlot = VRM.getStackSlot(VirtReg);
Well, according to VRM, %reg1631 is NOT assigned to a register (because it has
a remat id) even though linear scan assigned it one. VRM then returns garbage
as the stack slot and things go downhill from there.
So should VirtRegMap reset the stack slot and remat id maps when it is told to
assign a phys to a virt?
-Dave
On Tuesday 27 May 2008 19:36, David Greene wrote:> So should VirtRegMap reset the stack slot and remat id maps when it is told > to assign a phys to a virt?And what happens when SimpleSpiller needs to spill something that LiveIntervals has given a remat id? SimpleSpiller doesn't do remat and the stack slot will also be garbage in this case. -Dave
On May 27, 2008, at 5:36 PM, David Greene wrote:> I've been playing around with spillers and found that the > SimpleSpiller fails > badly on a particular code. > > The problem arises because SimpleSpiller does the test > VRM.isAssignedReg(virtReg) which is implemented as: > > 00183 bool isAssignedReg(unsigned virtReg) const { > 00184 if (getStackSlot(virtReg) == NO_STACK_SLOT && > 00185 getReMatId(virtReg) == NO_STACK_SLOT) > 00186 return true; > 00187 // Split register can be assigned a physical register as > well as a > 00188 // stack slot or remat id. > 00189 return (Virt2SplitMap[virtReg] && Virt2PhysMap[virtReg] !> NO_PHYS_REG); > 00190 }This is poorly named. All vr's will be assigned a physical register even if they are spilled or remat'd. This really should be isNotSpilledOrReMated. But then the exception is split register. Yeah, I know this is a mess. We are planning a complete rewrite.> > > VRM::assignVirt2Phys is implemented as: > > 00147 void assignVirt2Phys(unsigned virtReg, unsigned physReg) { > 00148 assert(TargetRegisterInfo::isVirtualRegister(virtReg) && > 00149 TargetRegisterInfo::isPhysicalRegister(physReg)); > 00150 assert(Virt2PhysMap[virtReg] == NO_PHYS_REG && > 00151 "attempt to assign physical register to already > mapped " > 00152 "virtual register"); > 00153 Virt2PhysMap[virtReg] = physReg; > 00154 } > > Note that this doesn't clear the stack slot or remat mappings. > > In the particular code I'm looking at, linear scan spills an > interval and > one of the intervals created (%reg1631) gets mark as > rematerializable by > LiveIntervals: > > 00984 if (CreatedNewVReg) { > 00985 if (DefIsReMat) { > 00986 vrm.setVirtIsReMaterialized(NewVReg, ReMatDefMI/*, > CanDelete*/); > 00987 if (ReMatIds[VNI->id] == VirtRegMap::MAX_STACK_SLOT) { > 00988 // Each valnum may have its own remat id. > 00989 ReMatIds[VNI->id] = vrm.assignVirtReMatId(NewVReg); > > Linear scan dutifully assigns %reg1631 to AL. Then SimpleSpiller > runs and > does this: > > 00261 unsigned VirtReg = MO.getReg(); > 00262 unsigned PhysReg = VRM.getPhys(VirtReg); > 00263 if (!VRM.isAssignedReg(VirtReg)) { > 00264 int StackSlot = VRM.getStackSlot(VirtReg); > > Well, according to VRM, %reg1631 is NOT assigned to a register > (because it has > a remat id) even though linear scan assigned it one. VRM then > returns garbage > as the stack slot and things go downhill from there. > > So should VirtRegMap reset the stack slot and remat id maps when it > is told to > assign a phys to a virt?No, every vr is assigned a physical register. It needs to know how what physical register to reload it, for example. Looks like SimpleSpiller has bit-rotted. There really isn't a good reason to fix it. Unless you want to use it, I'll just remove it. Evan> > > -Dave > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
On Friday 30 May 2008 15:13, Evan Cheng wrote:> No, every vr is assigned a physical register. It needs to know how > what physical register to reload it, for example. Looks like > SimpleSpiller has bit-rotted. There really isn't a good reason to fix > it. Unless you want to use it, I'll just remove it.No, don't remove it! It's a valuable debugging tool. I've got it working now but am in the middle of hunting this bug I found. Once things have stabilized here I'll see about submitting the fix. -Dave