Patchwork 0007-Add-open-ended-comments.patch

login
register
mail settings
Submitter Maxim Kuvyrkov
Date June 16, 2010, 3:59 p.m.
Message ID <4C18F4F4.606@codesourcery.com>
Download mbox | patch
Permalink /patch/55908/
State New
Headers show

Comments

Maxim Kuvyrkov - June 16, 2010, 3:59 p.m.
This patch adds several open-ended comments in gcse.c.  I'll be happy if 
anyone can answer some of them, in which case I'll check in the answers, 
rather than questions :).

Thank you,
Jeff Law - June 16, 2010, 5:18 p.m.
> This patch adds several open-ended comments in gcse.c.  I'll be happy 
> if anyone can answer some of them, in which case I'll check in the 
> answers, rather than questions :).
>
> Thank you,
>
@@ -3431,7 +3431,9 @@ process_insert_insn (struct expr *expr)

     For PRE, we want to verify that the expr is either transparent
     or locally anticipatable in the target block.  This check makes
-   no sense for code hoisting.  */
+   no sense for code hoisting.
+   ??? We always call this function with (PRE == 0), which makes the checks
+   useless.  */
See pre_edge_insert and search for EDGE_ABNORMAL.

That code went through several iterations and may no longer be 
necessary.  So we really should extend the existing comment before the 
call to insert_insn_end_basic_block from pre_edge_insert.

@@ -3535,6 +3537,9 @@ insert_insn_end_basic_block (struct expr *expr, 
basic_block bb, int pre)
    else
      new_insn = emit_insn_after_noloc (pat, insn, bb);

+  /* ??? It maybe useful to try set REG_EQUAL note on NEW_INSN here.
+     How can we do it?  */
+

Why do you think this is important?  Have you run into cases where 
having the note showing an alternate form of the expression would have 
allowed further optimization?

So, when you find a hoistable expression that reaches from its new block 
to a dominated child block which also evaluates the expression, if the 
insn in the dominated child has a REG_EQUAL note, you might be able to 
copy it.  So record it into a variable in the loop over the dominated 
blocks.  At the end of that loop, copy the REG_EQUAL note to the new 
insn (which you'll need to record as well).    You may have to verify 
the note is safe to copy/move.  I haven't pondered that aspect at all.

    /* Walk over each basic block looking for potentially hoistable
-     expressions, nothing gets hoisted from the entry block.  */
+     expressions, nothing gets hoisted from the entry block.
+
+     ??? It maybe worthwhile to walk CFG in DFS order over the 
dominator tree.
+     One can imagine a case when a dominated block B is linked before
+     its dominator A, so if expressions were hoisted from blocks C and D,
+     which B (and A) dominates, then it may occur that we miss
+     an optimization of moving these expressions all the way to A.
+     Alternatively, we may handle this case by updating expressions'
+     occurences to include instructions emitted by code hoisting, i.e.,
+     an expression emitted at the end of B will then be hoisted to A.  */
Well, yea, I guess this is possible.  I'm not sure if it happens much in 
practice.   I think your best bet would be controlling the order of 
blocks visited.  Trying to update the tables on the fly seems like it's 
going to get ugly quick.

Jeff

Patch

diff --git a/gcc/gcse.c b/gcc/gcse.c
index c81d71c..7215063 100644
--- a/gcc/gcse.c
+++ b/gcc/gcse.c
@@ -3431,7 +3431,9 @@  process_insert_insn (struct expr *expr)
 
    For PRE, we want to verify that the expr is either transparent
    or locally anticipatable in the target block.  This check makes
-   no sense for code hoisting.  */
+   no sense for code hoisting.
+   ??? We always call this function with (PRE == 0), which makes the checks
+   useless.  */
 
 static void
 insert_insn_end_basic_block (struct expr *expr, basic_block bb, int pre)
@@ -3535,6 +3537,9 @@  insert_insn_end_basic_block (struct expr *expr, basic_block bb, int pre)
   else
     new_insn = emit_insn_after_noloc (pat, insn, bb);
 
+  /* ??? It maybe useful to try set REG_EQUAL note on NEW_INSN here.
+     How can we do it?  */
+
   while (1)
     {
       if (INSN_P (pat))
@@ -4343,7 +4348,16 @@  hoist_code (void)
       index_map[expr->bitmap_index] = expr;
 
   /* Walk over each basic block looking for potentially hoistable
-     expressions, nothing gets hoisted from the entry block.  */
+     expressions, nothing gets hoisted from the entry block.
+
+     ??? It maybe worthwhile to walk CFG in DFS order over the dominator tree.
+     One can imagine a case when a dominated block B is linked before
+     its dominator A, so if expressions were hoisted from blocks C and D,
+     which B (and A) dominates, then it may occur that we miss
+     an optimization of moving these expressions all the way to A.
+     Alternatively, we may handle this case by updating expressions'
+     occurences to include instructions emitted by code hoisting, i.e.,
+     an expression emitted at the end of B will then be hoisted to A.  */
   FOR_EACH_BB (bb)
     {
       int found = 0;