[Beignet] [PATCH 2/2] Skip spill/unspill instruction when trying to do spill.

[Zhigang Gong]

After discussion with Ruiling, now I know that I made a mistake when I reviewed the original patch
as below:

> > -          if(selReg.file == GEN_GENERAL_REGISTER_FILE && reg == spilledReg) {
> > +          if(reg == spilledReg && selReg.file == GEN_GENERAL_REGISTER_FILE && selReg.physical == 0) {

It's checking whether the selReg is a physical or virtual register. And the selReg only affect the
gen registers in this instruction and will not affect the other instructions. So the following code:
dst.physical =1; dst.nr<http://dst.nr> = dstStart+dstID; dst.subnr = 0;
will not affect any other instruction, thus this patch will not bring any side effect.

The only tricky thing in this patch is, for a directly created physical register which is not created
at the virtual register layer, it doesn't have a valid virtual register thus it may cause some random
consequent behaviour and need to be handled carefully. Spill/Unspill introduce some of this kind of
registers and met this type of bug, and this patch is to fix that issue.

This patch LGTM, and will push it latter. Thanks Ruiling.


On Mon, Aug 12, 2013 at 02:46:23AM +0000, Song, Ruiling wrote:
> What spillReg() will do is like below:
> Find ALL references to the virtual register [spilledReg], and add unspill instruction if spilledReg is used as src, and add spill instruction if used ad dst.
> So, when finding the virtual register, of course I need to skip the physical register, as I was finding the virtual register.
> The bug was because for physical register, the value.reg was not initialized. The value.reg in physical register is undefined.
> 
> " The skip here means it will use the physical registers at the previous time it was spilled/unspiled. Is this right?"
> This is not right, skip means when I am searching for a virtual register which I need to spill, I should skip a physical register as it is not what I want.
> it will NOT use the same physical registers at the previous time it was spilled/unspilled.
> 
> Which physical register it will use depends on the position in the instruction: if used as src, use(srcStart+srcID, 0,). If used as dst in the instruction, it will use (dstStart + dstID, 0,).
> All spilled virtual registers will use same set of physical register: that is the reserved register pool. But why you have concern on this? Register is only valid in one instruction, in another instruction, all the data will be read from/ written to scratch memory. So, the only thing need to keep is: in the same instruction, different src virtual register should map to different physical register. Different dst virtual register should map to different physical register.
> Then everything will work right.
> 
> -----Original Message-----
> From: Zhigang Gong [mailto:zhigang.gong at linux.intel.com] 
> Sent: Monday, August 12, 2013 10:26 AM
> To: Song, Ruiling
> Cc: zhigang gong; beignet at lists.freedesktop.org
> Subject: Re: [Beignet] [PATCH 2/2] Skip spill/unspill instruction when trying to do spill.
> 
> Thanks for the explaination. But I'm still not convinced.
> The keypoint is that this patch will skip those already spilled/unspilled registers. The skip here means it will use the physical registers at the previous time it was spilled/unspiled. Is this right?
> 
> If so, then all of the concequent reference to these different virtual registers will use the same set of reserved registers.
> That's my concern.
> 
> If you don't skip those registers in spilled/unspill function, I will be fine with your explaination.
> 
> Any further comments?
> 
> On Mon, Aug 12, 2013 at 01:45:27AM +0000, Song, Ruiling wrote:
> > Good question, let me explain a little more.
> > For a spilled virtual register, it will not have a dedicated physical register for it anymore. And its real data lies in scratch memory.
> > When a virtual register is used, it will be always reloaded from scratch memory. when it is updated, it will be written to scratch memory immediately.
> > The content of the reserved register is only valid for one instruction(not including spill/unspill instruction). In the next instruction, it will be used to store other data.
> > 
> > From: zhigang gong [mailto:zhigang.gong at gmail.com]
> > Sent: Friday, August 09, 2013 5:02 PM
> > To: Song, Ruiling
> > Cc: beignet at lists.freedesktop.org
> > Subject: Re: [Beignet] [PATCH 2/2] Skip spill/unspill instruction when trying to do spill.
> > 
> > On Fri, Aug 9, 2013 at 4:38 PM, Song, Ruiling <ruiling.song at intel.com<mailto:ruiling.song at intel.com>> wrote:
> > When I do a spill, I directly construct a physical register, see below:
> > spill->src(0) =GenRegister(GEN_GENERAL_REGISTER_FILE, dstStart + 
> > spill->dstID, 0,
> >                                               selReg.type, 
> > selReg.vstride, selReg.width, selReg.hstride); and also allocate one physical register from reserved register pool, change the spilled virtual register to physical register:
> > dst.physical =1; dst.nr<http://dst.nr> = dstStart+dstID; dst.subnr = 
> > 0; so, in later emit code, when they call ra->genReg(), I will directly return the physical register; see GenRegAllocator::Opaque::genReg()
> >   Sounds good. But if my understanding is correct, then after spill or unspill, the spilledReg will become a physical
> >   register:
> >             // change nr/subnr, keep other register settings
> >            src.nr<http://src.nr> = srcStart+srcID; src.subnr=0; 
> > src.physical=1;
> > 
> >             // change nr/subnr, keep other register settings
> >             dst.physical =1; dst.nr<http://dst.nr> = dstStart+dstID; dst.subnr = 0;
> >    And the most important side effect is that the reserved registers will be used.
> >    Then latter, if we want to spill another register, then it may use the reserved registers again but those registers are already used by the
> >    previous spilled register.
> > 
> >    Is it a problem?
> > 
> > 
> > From: 
> > beignet-bounces+ruiling.song=intel.com at lists.freedesktop.org<mailto:in
> > tel.com at lists.freedesktop.org> 
> > [mailto:beignet-bounces+ruiling.song<mailto:beignet-bounces%2Bruiling.
> > song>=intel.com at lists.freedesktop.org<mailto:intel.com at lists.freedeskt
> > op.org>] On Behalf Of zhigang gong
> > Sent: Friday, August 09, 2013 4:29 PM
> > To: Song, Ruiling
> > Cc: 
> > beignet at lists.freedesktop.org<mailto:beignet at lists.freedesktop.org>
> > Subject: Re: [Beignet] [PATCH 2/2] Skip spill/unspill instruction when trying to do spill.
> > 
> > Could you tell something more about how could a spilledReg be a physical register?
> > This is not very clear for me. If a virtual register is to be spilled, 
> > then it will always be a virtual register, and will be unspilled when 
> > it is used. When it may become a physical register?
> > 
> > 
> > On Fri, Aug 9, 2013 at 1:23 PM, Ruiling Song <ruiling.song at intel.com<mailto:ruiling.song at intel.com>> wrote:
> > We can only spill virtual registers, should skip physical register.
> > This fix random failure of compiler_box_blur when do spilling.
> > 
> > Signed-off-by: Ruiling Song 
> > <ruiling.song at intel.com<mailto:ruiling.song at intel.com>>
> > ---
> >  backend/src/backend/gen_insn_selection.cpp |    7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/backend/src/backend/gen_insn_selection.cpp 
> > b/backend/src/backend/gen_insn_selection.cpp
> > index 3610051..5cafa98 100644
> > --- a/backend/src/backend/gen_insn_selection.cpp
> > +++ b/backend/src/backend/gen_insn_selection.cpp
> > @@ -628,12 +628,15 @@ namespace gbe
> > 
> >      for (auto &block : blockList)
> >        for (auto &insn : block.insnList) {
> > +        // spill / unspill insn should be skipped when do spilling
> > +        if(insn.opcode == SEL_OP_SPILL_REG || insn.opcode == 
> > + SEL_OP_UNSPILL_REG) continue;
> > +
> >          const uint32_t srcNum = insn.srcNum, dstNum = insn.dstNum;
> > 
> >          for (uint32_t srcID = 0; srcID < srcNum; ++srcID) {
> >            const GenRegister selReg = insn.src(srcID);
> >            const ir::Register reg = selReg.reg();
> > -          if(selReg.file == GEN_GENERAL_REGISTER_FILE && reg == spilledReg) {
> > +          if(reg == spilledReg && selReg.file == 
> > + GEN_GENERAL_REGISTER_FILE && selReg.physical == 0) {
> >              GBE_ASSERT(srcID < 5);
> >              SelectionInstruction *unspill = this->create(SEL_OP_UNSPILL_REG, 1, 0);
> >              unspill->state  = GenInstructionState(simdWidth); @@ 
> > -652,7 +655,7 @@ namespace gbe
> >          for (uint32_t dstID = 0; dstID < dstNum; ++dstID) {
> >            const GenRegister selReg = insn.dst(dstID);
> >            const ir::Register reg = selReg.reg();
> > -          if(selReg.file == GEN_GENERAL_REGISTER_FILE && reg == spilledReg) {
> > +          if(reg == spilledReg && selReg.file == 
> > + GEN_GENERAL_REGISTER_FILE && selReg.physical == 0) {
> >              GBE_ASSERT(dstID < 5);
> >              SelectionInstruction *spill = this->create(SEL_OP_SPILL_REG, 0, 1);
> >              spill->state  = GenInstructionState(simdWidth);
> > --
> > 1.7.9.5
> > 
> > _______________________________________________
> > Beignet mailing list
> > Beignet at lists.freedesktop.org<mailto:Beignet at lists.freedesktop.org>
> > http://lists.freedesktop.org/mailman/listinfo/beignet
> > 
> > 
> 
> > _______________________________________________
> > Beignet mailing list
> > Beignet at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/beignet
> 
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet