[Beignet] [PATCH V2 3/3] add local copy propagation optimization for each basic block

[Zhigang Gong]

On Thu, Sep 24, 2015 at 02:58:22AM +0000, Guo, Yejun wrote:
> 
> 
> -----Original Message-----
> From: Zhigang Gong [mailto:zhigang.gong at linux.intel.com] 
> Sent: Thursday, September 24, 2015 9:14 AM
> To: Guo, Yejun
> Cc: beignet at lists.freedesktop.org
> Subject: Re: [Beignet] [PATCH V2 3/3] add local copy propagation optimization for each basic block
> 
>   
> > +  void SelBasicBlockOptimizer::propagateRegister(GenRegister& dst, 
> > + const GenRegister& src)
> This function looks much like a GenRegister static method, because it doesn't reference any external information.
> 
> [yejun] Yes, it is. I wrote here because I'm not sure if there will be other changes due to regression issue or for extended usage.  I'm ok to change to GenRegister static method.
> > +  {
> > +    dst.type = src.type;
> > +    dst.file = src.file;
> > +    dst.physical = src.physical;
> > +    dst.subphysical = src.subphysical;
> > +    dst.value.reg = src.value.reg;
> > +    dst.vstride = src.vstride;
> > +    dst.width = src.width;
> > +    dst.hstride = src.hstride;
> > +    dst.quarter = src.quarter;
> > +    dst.nr = src.nr;
> > +    dst.subnr = src.subnr;
> > +    dst.address_mode = src.address_mode;
> > +    dst.a0_subnr = src.a0_subnr;
> > +    dst.addr_imm = src.addr_imm;
> > +
> > +    dst.negation = dst.negation ^ src.negation;
> > +    dst.absolute = dst.absolute | src.absolute;  }
> > +
> 
> 
> 
> > +
> > +  void SelBasicBlockOptimizer::changeInsideReplaceInfos(const 
> > + SelectionInstruction& insn, GenRegister& var)  {
> > +    for (ReplaceInfo* info : replaceInfos) {
> > +      if (info->intermedia.reg() == var.reg()) {
A new comment here is that the above loop is a little bit too heavy.
For a large BB which has many MOV instructions, it will iterate too
many times for instructions after those MOV instructions. A better
way is to change to use a new map type of replaceInfos:
map <ir::Register, set<ReplaceInfo*>>

This will be much faster than iterate all infos for each instruction.

> > +        bool replaceable = false;
> It's better to add a comment here to describe the cases which can't be replaced and why.
> 
> [yejun] actually, I think the code itself explains something, it is much complex to explain every detail in human words. I consider it as a nice-to-have since the basic logic is simple.
I still think it is not that simple. A case just came into my mind which we can't do replacement is:

MOV %r0, %r1
ADD %r1, %r1, 1
...
ADD %r10, %r0, 1
...

I'm afraid that your current code can't deal it correctly, right?

> > +        if (insn.opcode != SEL_OP_BSWAP && !insn.isWrite()) {
> > +          if (!info->replacementChanged && info->intermedia.type == var.type && info->intermedia.quarter == var.quarter && info->intermedia.subnr == var.subnr) {
> 
> 
> 
> 
> > +            uint32_t elements = CalculateElements(var, insn.state.execWidth);
> > +            if (info->elements == elements) {
> Why not compare each attribute value, such as vstride_size,hstride_size,.....?
> 
> [Yejun] the execWidth is not recorded in the GenRegister, and I once saw instructions such as:
> mov(1)  dst (vstride_size or hstride_size is 1/2/... or something else), src
> mov(16) anotherdst, dst (with stride as 0)
> 
> the dst here is the same, but the strides are different, to handle this case, I add the function CalculateElements.
The example you gave here has two different GenRegisters as GenRegister
has vstride and hstride members. Right? My previous comment suggests to
compare two GenRegistes' attributs and I can't understand your point here.


Thanks,
Zhigang Gong.

> 
> 
> 
> 
> The other parts LGTM,
> 
> Thanks,
> Zhigang Gong
> > +              info->toBeReplacements.insert(&var);
> > +              replaceable = true;
> > +            }
> > +          }
> > +        }
> > +
> > +        //if we found the same register, but could not be replaced for some reason,
> > +        //that means we could not remove MOV instruction, and so no replacement,
> > +        //so we'll remove the info for this case.
> > +        if (!replaceable) {
> > +          replaceInfos.erase(info);
> > +          delete info;
> > +        }
> > +        break;
> > +      }
> > +    }
> > +  }