Re: [tree-ssa] PATCH to gimplify_boolean_expr

[law at redhat dot com]

In message <wvlbrud9xsk.fsf@prospero.boston.redhat.com>, Jason Merrill writes:
 >.gimple:
 >
 >  F.1 = t1->common.code;
 >  F.2 = (unsigned char)F.1;
 >  F.3 = F.2 == 0;
 >  F.4 = t2 != 0B;
 >  T.5 = F.4;
 >  if (T.5)
 >    {
 >      F.6 = t2->common.code;
 >      F.7 = (unsigned char)F.6;
 >      F.8 = F.7 == 0;
 >      T.5 = F.8;
 >    }
 >  else
 >    {
 >      (void)0
 >    };
 >  F.9 = F.3 ^ T.5;
 >
 >.dom:
 >
 >  F.1_6 = t1_4->common.code;
 >  F.2_7 = (unsigned char)F.1_6;
 >  F.3_8 = F.2_7 == 0;
 >  F.4_10 = t2_9 != 0B;
 >  T.5_11 = F.4_10;
 >  if (F.4_10)
 >    {
 >      # BLOCK 1 (/home/jason/src/ast/gcc/gcc/testsuite/gcc.dg/tree-ssa/200307
 >28-1.c:36).  PRED: 0.  SUCC: 3.
 >      F.6_12 = t2_9->common.code;
 >      F.7_13 = (unsigned char)F.6_12;
 >      F.8_14 = F.7_13 == 0;
 >      T.5_15 = F.8_14;
 >    }
 >  else
 >    {
 >      # BLOCK 2.  PRED: 0.  SUCC: 3.
 >      (void)0
 >    };
 >  
 >  # BLOCK 3 (/home/jason/src/ast/gcc/gcc/testsuite/gcc.dg/tree-ssa/20030728-1
 >.c:36).  PRED: 2 1.  SUCC: 5 4.
 >  #   T.5_1 = PHI <F.8_14(1), F.4_10(2)>;
 >  F.9_16 = F.3_8 ^ T.5_1;
 >
 >In dom, we replaced the conditional use of T.5 with F.4, so we introduce
 >F.4_10==true and F.4_10==false in the arms rather than do the same to T.5.
 >When we re-unify T.5 in block 3, the last value we have for T.5 on the
 >false branch is F.4_10.
 >
 >If we had not done copy prop here, we would have set up the constant values
 >for T.5 instead, and the false one would have flowed into the PHI.
 >
 >Recursing to look up the constant value of F.4_10 in block 2 would also fix
 >this case.
Sorry, I still don't see why we're not getting the desired propagation.
Maybe I'm being dense at 1:30am :-)

Recording F.4_10 == false for the ELSE arm should have propagated the value
false (zero) into the use of F.4_10 in the PHI node at the unification point.
The only reason I can think that didn't happen would be a type difference.

There's not much you can really do for F.8_14(1) in the PHI node though.

So what the copy propagation really does is turn a copy from one reg to another
into assigning the register the value zero in the out-of-ssa pass.  It doesn't
allow us to do anything particularly interesting right now with the
assignment to F.9_16.

 >> As long as "foo" and "bar" are temporaries, I don't mind.  One of the
 >> problems we've got right now is that we can have nontrivial operands 
 >> inside COND_EXPR_COND.  That inhibits redundant expression elimination.
 >
 >We can?  Testcase, please.  The operands are supposed to be vals.  Of
 >course, currently memory variables can be vals, but I'm working on fixing
 >that.
You hit the nail on the head.  The cases I've seen have involved objects in
static storage.  For example:

gcc.dg/tree-ssa/20030807-1.c (currently failing):


struct rtx_def;
typedef struct rtx_def *rtx;



union rtunion_def
{
  int rtint;
};
typedef union rtunion_def rtunion;



struct rtx_def
{
  rtunion fld[1];

};

static int *uid_cuid;
static int max_uid_cuid;

static void
bar ()
{

  rtx place = 0;

  if (place->fld[0].rtint <= max_uid_cuid
      && (place->fld[0].rtint > max_uid_cuid ? insn_cuid (place) :
          uid_cuid[place->fld[0].rtint]))
    ;
}


Which generates the following .ssa dump:

;; Function bar (bar)

bar ()
{
  int T.1;
  _Bool iftmp.2;
  _Bool iftmp.3;
  int T.4;
  _Bool iftmp.5;
  unsigned int T.6;
  unsigned int T.7;
  int * T.8;
  int * T.9;
  int T.10;
  struct rtx_def * place;
  extern  insn_cuid;

  place_6 = 0B;
  T.1_8 = 0B->fld[0].rtint;
  if (T.1_8 <= max_uid_cuid)
    {
      T.1_9 = T.1_8;
      if (T.1_8 > max_uid_cuid)
        {
          T.4_10 = insn_cuid (0B);
          if (T.4_10 != 0)
            {
              iftmp.3_12 = 1
            }
          else
            {
              iftmp.3_13 = 0
            };
          iftmp.2_14 = iftmp.3_2;
        }
      else
        {
          T.1_16 = T.1_8;
          T.6_17 = (unsigned int)T.1_8;
          T.7_18 = T.6_17 * 4;
          T.8_19 = (int *)T.7_18;
          T.9_20 = uid_cuid + T.8_19;
          T.10_21 = *T.9_20;
          if (T.10_21 != 0)
            {
              iftmp.5_22 = 1
            }
          else
            {
              iftmp.5_23 = 0
            };
          iftmp.2_24 = iftmp.5_4;
        };
      (void)0;
    }
  else
    {
      (void)0
    };
}   

Note the two if conditionals involving max_uid_cuid, a variable in
static storage.  If max_uid_cuid was first loaded into a temporary
than all the right things happen and we can remove the second test
using max_uid_cuid.


 >>  >Another fix is to teach dom to propagate conditional information back up
 >>  >the def chain; I'll have a preliminary patch for that shortly.
 >> Now that would be good.   I've seen some tests where that could eliminate
 >> some useless crud -- particuliarly where we lose information due to 
 >> casting between different types.
 >
 >I've attached what I have below; feel free to run with it if you're
 >interested, I won't do anything more with it for a while.
I doubt I'll be able to do anything with it this week.

FWIW, my work in forcing loads of such variables into temporaries by changing
one of the is_blah_blah predicates stalled due to a regression in the gcov
tests -- the counts on a particular expression were off and I've been unable
to return to it and sort through the gcov output.