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

[Zhigang Gong]

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 + 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:intel.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.freedesktop.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