[jira] Commented: (HARMONY-5580) [drlvm][jitrino][opt][perf] branch translator enhancement

["George Timoshenko (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HARMONY-5580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12577780#action_12577780 ] 

George Timoshenko commented on HARMONY-5580:
--------------------------------------------

Dmitry,

My notes are mostly related to the design.
I see, you are basing your optimization on the existing code in Branch Translator.
I'd like you to improve the design of all parts of BT which are similar to yours.

1. Tabs into spaces.

2. I'd suggest:

+            if (!bb->isBlockNode() || bb->isEmpty())
+                continue;
instead of:
+            if (bb->isBlockNode()){
+                if(bb->isEmpty())
+                    continue;

3. Similarly:

+                Inst * inst = (Inst *)bb->getLastInst();
+                assert(inst); // bb must be non empty here, right?
+                //skip if the last instruction in basic block is not a conditional branch instruction
+                if( !inst->hasKind(Inst::Kind_BranchInst) || bb->getOutEdges().size() != 1) {
+                    continue;
+                }

is more compact and clear. And it does not produce additional indention.

+                Inst * inst = (Inst *)bb->getLastInst();
+                //check is last instruction in basic block is a conditional branch instruction
+                if(inst && inst->hasKind(Inst::Kind_BranchInst)) {
+                    //get successors of bb
+                    if(bb->getOutEdges().size() == 1)
+                        continue;

4. I'd also suggest to rename inst and prevInst to brInst and cmpInst.

5. Do we really need these changes?

-    if (cmovs) {
+    if (cmovs) {
         for (Nodes::const_reverse_iterator it = nodes.rbegin(),end = nodes.rend();it!=end; ++it) {
             Node* bb = *it;
             if (bb->isBlockNode()){
@@ -342,14 +441,16 @@
 
                     if(bb->getOutEdges().size() == 1)
                         continue;
 
-                    Node * trueBB = bb->getTrueEdge()->getTargetNode();
-                    Node * falseBB = bb->getFalseEdge()->getTargetNode();
+                    Edge * trueEdge = bb->getTrueEdge();
+                    Edge * falseEdge = bb->getFalseEdge();
+                    Node * trueBB = trueEdge->getTargetNode();
+                    Node * falseBB = falseEdge->getTargetNode();
 
                     ConditionMnemonic condMnem = ConditionMnemonic(inst->getMnemonic() - getBaseConditionMnemonic(inst->getMnemonic()));
 
-                    //check is both successors have only instruction
                     Inst * trueInst = (Inst *)trueBB->getFirstInst();
                     Inst * falseInst = (Inst *)falseBB->getFirstInst();
+                    Inst * prevInst = inst->getPrevInst();
                     if(trueBB && falseInst && trueBB->getInstCount() == 1 && falseBB->getInstCount() == 1 && trueInst->getMnemonic() == Mnemonic_MOV && falseInst->getMnemonic() == Mnemonic_MOV && trueInst->getOpnd(0) == falseInst->getOpnd(0) && trueInst->getOpnd(0)->getMemOpndKind() == MemOpndKind_Null) {
                         //check is bb is only predecessor for trueBB and falseBB
                         bool canBeRemoved = true;
@@ -401,7 +502,6 @@
 
                             inst->unlink();
                             irManager->getFlowGraph()->removeNode(falseBB);
                             irManager->getFlowGraph()->removeNode(trueBB);
-                            
                     }
                 } 
             }//end if BasicBlock
 

 



> [drlvm][jitrino][opt][perf] branch translator enhancement
> ---------------------------------------------------------
>
>                 Key: HARMONY-5580
>                 URL: https://issues.apache.org/jira/browse/HARMONY-5580
>             Project: Harmony
>          Issue Type: Improvement
>          Components: DRLVM
>         Environment: IA32
>            Reporter: Dmitry Pronichkin
>            Assignee: Mikhail Fursov
>         Attachments: btr.patch
>
>
> The patch introduces new way of branch elimination. It eliminates branches like this:
> if (x<0) {
>     x+=y;
> }
> transforming them in following assembly:
> mov tmp, x
> sar tmp, 31  (spreading sign for the whole value)
> and tmp, y    (moving "y" in tmp if x<0)
> add x, tmp

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.