GBE: Fix a bug in phicopy coaleasing.

Submitted by Song, Ruiling on July 16, 2015, 8:40 a.m.

Details

Message ID 1437036033-8783-1-git-send-email-ruiling.song@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Song, Ruiling July 16, 2015, 8:40 a.m.
we cannot simply 'break', the isOpt is not set correctly when break.
let's use if() check which will not interfere with the old isOpt logic.

Signed-off-by: Ruiling Song <ruiling.song@intel.com>
---
 backend/src/llvm/llvm_gen_backend.cpp | 89 +++++++++++++++++------------------
 1 file changed, 44 insertions(+), 45 deletions(-)

Patch hide | download patch | download mbox

diff --git a/backend/src/llvm/llvm_gen_backend.cpp b/backend/src/llvm/llvm_gen_backend.cpp
index 0ec113d..1e141cd 100644
--- a/backend/src/llvm/llvm_gen_backend.cpp
+++ b/backend/src/llvm/llvm_gen_backend.cpp
@@ -2191,55 +2191,54 @@  namespace gbe
         const ir::Register phiCopySrc = phiCopyDefInsn->getSrc(0);
         const ir::UseSet *phiCopySrcUse = dag->getRegUse(phiCopySrc);
         const ir::DefSet *phiCopySrcDef = dag->getRegDef(phiCopySrc);
-        // non-ssa value. skip opt on such kind of value
-        if (phiCopySrcDef->size() > 1) break;
-        // we can only coalesce instruction dest
-        if ((*(phiCopySrcDef->begin()))->getType() != ValueDef::DEF_INSN_DST) break;
-
-        const ir::Instruction *phiCopySrcDefInsn = (*(phiCopySrcDef->begin()))->getInstruction();
-        if(bb == phiDefBB && bb == phiCopySrcDefInsn->getParent()) {
-          // phiCopy, phiCopySrc defined in same basicblock as phi
-          // try to coalease phiCopy and phiCopySrc first.
-          // consider below situation:
-          // bb1:
-          //    ...
-          // bb2:
-          //    x = phi [x1, bb1], [x2, bb2]
-          //    x2 = x+1;
-          // after de-ssa:
-          // bb2:
-          //    mov x, x-copy
-          //    add x2, x, 1
-          //    mov x-copy, x2
-          //  obviously x2, x-copy and x2 can be mapped to same virtual register
-
-          ir::BasicBlock::const_iterator iter = ir::BasicBlock::const_iterator(phiCopySrcDefInsn);
-          ir::BasicBlock::const_iterator iterE = bb->end();
-          // check no use of phi in this basicblock between [phiCopySrc def, bb end]
-          bool phiPhiCopySrcInterfere = false;
-          while (iter != iterE) {
-            const ir::Instruction *insn = iter.node();
-            // check phiUse
-            for (unsigned i = 0; i < insn->getSrcNum(); i++) {
-              ir::Register src = insn->getSrc(i);
-              if (src == phi) {
-                phiPhiCopySrcInterfere = true; break;
+
+        // we should only do coaleasing on instruction-def and ssa-value
+        if (phiCopySrcDef->size() == 1 && (*(phiCopySrcDef->begin()))->getType() == ValueDef::DEF_INSN_DST) {
+          const ir::Instruction *phiCopySrcDefInsn = (*(phiCopySrcDef->begin()))->getInstruction();
+          if(bb == phiDefBB && bb == phiCopySrcDefInsn->getParent()) {
+            // phiCopy, phiCopySrc defined in same basicblock as phi
+            // try to coalease phiCopy and phiCopySrc first.
+            // consider below situation:
+            // bb1:
+            //    ...
+            // bb2:
+            //    x = phi [x1, bb1], [x2, bb2]
+            //    x2 = x+1;
+            // after de-ssa:
+            // bb2:
+            //    mov x, x-copy
+            //    add x2, x, 1
+            //    mov x-copy, x2
+            //  obviously x2, x-copy and x2 can be mapped to same virtual register
+
+            ir::BasicBlock::const_iterator iter = ir::BasicBlock::const_iterator(phiCopySrcDefInsn);
+            ir::BasicBlock::const_iterator iterE = bb->end();
+            // check no use of phi in this basicblock between [phiCopySrc def, bb end]
+            bool phiPhiCopySrcInterfere = false;
+            while (iter != iterE) {
+              const ir::Instruction *insn = iter.node();
+              // check phiUse
+              for (unsigned i = 0; i < insn->getSrcNum(); i++) {
+                ir::Register src = insn->getSrc(i);
+                if (src == phi) {
+                  phiPhiCopySrcInterfere = true; break;
+                }
               }
+              ++iter;
             }
-            ++iter;
-          }
-          if (!phiPhiCopySrcInterfere) {
-            // phiCopy source can be coaleased with phiCopy
-            const_cast<Instruction *>(phiCopyDefInsn)->remove();
+            if (!phiPhiCopySrcInterfere) {
+              // phiCopy source can be coaleased with phiCopy
+              const_cast<Instruction *>(phiCopyDefInsn)->remove();
 
-            for (auto &s : *phiCopySrcDef) {
-              const Instruction *phiSrcDefInsn = s->getInstruction();
-              replaceDst(const_cast<Instruction *>(phiSrcDefInsn), phiCopySrc, phiCopy);
-            }
+              for (auto &s : *phiCopySrcDef) {
+                const Instruction *phiSrcDefInsn = s->getInstruction();
+                replaceDst(const_cast<Instruction *>(phiSrcDefInsn), phiCopySrc, phiCopy);
+              }
 
-            for (auto &s : *phiCopySrcUse) {
-              const Instruction *phiSrcUseInsn = s->getInstruction();
-              replaceSrc(const_cast<Instruction *>(phiSrcUseInsn), phiCopySrc, phiCopy);
+              for (auto &s : *phiCopySrcUse) {
+                const Instruction *phiSrcUseInsn = s->getInstruction();
+                replaceSrc(const_cast<Instruction *>(phiSrcUseInsn), phiCopySrc, phiCopy);
+              }
             }
           }
         }

Comments

This opt is independent of the original PhiCopy opt, maybe can use a separate phiCopyDef loop for clear.
Anyway, I push it first. Could refine it later.

> -----Original Message-----

> From: Beignet [mailto:beignet-bounces@lists.freedesktop.org] On Behalf Of

> Ruiling Song

> Sent: Thursday, July 16, 2015 16:41

> To: beignet@lists.freedesktop.org

> Cc: Song, Ruiling

> Subject: [Beignet] [PATCH] GBE: Fix a bug in phicopy coaleasing.

> 

> we cannot simply 'break', the isOpt is not set correctly when break.

> let's use if() check which will not interfere with the old isOpt logic.

> 

> Signed-off-by: Ruiling Song <ruiling.song@intel.com>

> ---

>  backend/src/llvm/llvm_gen_backend.cpp | 89 +++++++++++++++++---------

> ---------

>  1 file changed, 44 insertions(+), 45 deletions(-)

> 

> diff --git a/backend/src/llvm/llvm_gen_backend.cpp

> b/backend/src/llvm/llvm_gen_backend.cpp

> index 0ec113d..1e141cd 100644

> --- a/backend/src/llvm/llvm_gen_backend.cpp

> +++ b/backend/src/llvm/llvm_gen_backend.cpp

> @@ -2191,55 +2191,54 @@ namespace gbe

>          const ir::Register phiCopySrc = phiCopyDefInsn->getSrc(0);

>          const ir::UseSet *phiCopySrcUse = dag->getRegUse(phiCopySrc);

>          const ir::DefSet *phiCopySrcDef = dag->getRegDef(phiCopySrc);

> -        // non-ssa value. skip opt on such kind of value

> -        if (phiCopySrcDef->size() > 1) break;

> -        // we can only coalesce instruction dest

> -        if ((*(phiCopySrcDef->begin()))->getType() != ValueDef::DEF_INSN_DST)

> break;

> -

> -        const ir::Instruction *phiCopySrcDefInsn = (*(phiCopySrcDef->begin()))-

> >getInstruction();

> -        if(bb == phiDefBB && bb == phiCopySrcDefInsn->getParent()) {

> -          // phiCopy, phiCopySrc defined in same basicblock as phi

> -          // try to coalease phiCopy and phiCopySrc first.

> -          // consider below situation:

> -          // bb1:

> -          //    ...

> -          // bb2:

> -          //    x = phi [x1, bb1], [x2, bb2]

> -          //    x2 = x+1;

> -          // after de-ssa:

> -          // bb2:

> -          //    mov x, x-copy

> -          //    add x2, x, 1

> -          //    mov x-copy, x2

> -          //  obviously x2, x-copy and x2 can be mapped to same virtual register

> -

> -          ir::BasicBlock::const_iterator iter =

> ir::BasicBlock::const_iterator(phiCopySrcDefInsn);

> -          ir::BasicBlock::const_iterator iterE = bb->end();

> -          // check no use of phi in this basicblock between [phiCopySrc def, bb

> end]

> -          bool phiPhiCopySrcInterfere = false;

> -          while (iter != iterE) {

> -            const ir::Instruction *insn = iter.node();

> -            // check phiUse

> -            for (unsigned i = 0; i < insn->getSrcNum(); i++) {

> -              ir::Register src = insn->getSrc(i);

> -              if (src == phi) {

> -                phiPhiCopySrcInterfere = true; break;

> +

> +        // we should only do coaleasing on instruction-def and ssa-value

> +        if (phiCopySrcDef->size() == 1 && (*(phiCopySrcDef->begin()))-

> >getType() == ValueDef::DEF_INSN_DST) {

> +          const ir::Instruction *phiCopySrcDefInsn = (*(phiCopySrcDef-

> >begin()))->getInstruction();

> +          if(bb == phiDefBB && bb == phiCopySrcDefInsn->getParent()) {

> +            // phiCopy, phiCopySrc defined in same basicblock as phi

> +            // try to coalease phiCopy and phiCopySrc first.

> +            // consider below situation:

> +            // bb1:

> +            //    ...

> +            // bb2:

> +            //    x = phi [x1, bb1], [x2, bb2]

> +            //    x2 = x+1;

> +            // after de-ssa:

> +            // bb2:

> +            //    mov x, x-copy

> +            //    add x2, x, 1

> +            //    mov x-copy, x2

> +            //  obviously x2, x-copy and x2 can be mapped to same virtual register

> +

> +            ir::BasicBlock::const_iterator iter =

> ir::BasicBlock::const_iterator(phiCopySrcDefInsn);

> +            ir::BasicBlock::const_iterator iterE = bb->end();

> +            // check no use of phi in this basicblock between [phiCopySrc def, bb

> end]

> +            bool phiPhiCopySrcInterfere = false;

> +            while (iter != iterE) {

> +              const ir::Instruction *insn = iter.node();

> +              // check phiUse

> +              for (unsigned i = 0; i < insn->getSrcNum(); i++) {

> +                ir::Register src = insn->getSrc(i);

> +                if (src == phi) {

> +                  phiPhiCopySrcInterfere = true; break;

> +                }

>                }

> +              ++iter;

>              }

> -            ++iter;

> -          }

> -          if (!phiPhiCopySrcInterfere) {

> -            // phiCopy source can be coaleased with phiCopy

> -            const_cast<Instruction *>(phiCopyDefInsn)->remove();

> +            if (!phiPhiCopySrcInterfere) {

> +              // phiCopy source can be coaleased with phiCopy

> +              const_cast<Instruction *>(phiCopyDefInsn)->remove();

> 

> -            for (auto &s : *phiCopySrcDef) {

> -              const Instruction *phiSrcDefInsn = s->getInstruction();

> -              replaceDst(const_cast<Instruction *>(phiSrcDefInsn), phiCopySrc,

> phiCopy);

> -            }

> +              for (auto &s : *phiCopySrcDef) {

> +                const Instruction *phiSrcDefInsn = s->getInstruction();

> +                replaceDst(const_cast<Instruction *>(phiSrcDefInsn), phiCopySrc,

> phiCopy);

> +              }

> 

> -            for (auto &s : *phiCopySrcUse) {

> -              const Instruction *phiSrcUseInsn = s->getInstruction();

> -              replaceSrc(const_cast<Instruction *>(phiSrcUseInsn), phiCopySrc,

> phiCopy);

> +              for (auto &s : *phiCopySrcUse) {

> +                const Instruction *phiSrcUseInsn = s->getInstruction();

> +                replaceSrc(const_cast<Instruction *>(phiSrcUseInsn), phiCopySrc,

> phiCopy);

> +              }

>              }

>            }

>          }

> --

> 2.3.6

> 

> _______________________________________________

> Beignet mailing list

> Beignet@lists.freedesktop.org

> http://lists.freedesktop.org/mailman/listinfo/beignet