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

[Guo, Yejun]

-----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()) {
> +        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.
> +        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 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;
> +      }
> +    }
> +  }