Re: [PATCH] doloop_optimize miscompiles openssl

[Franz Sirl <Franz dot Sirl-kernel at lauterbach dot com>]

At 00:34 26.09.00, Geoff Keating wrote:

>Franz Sirl <Franz.Sirl-kernel@lauterbach.com> writes:
>
> >       * doloop.c (doloop_modify): Prevent delete_insn() from
> >       deleting too much.  Prefer loop->top over loop->start as
> >       target for the new JUMP insn.
> >       (doloop_valid_p): Ignore loop with exit_count > 1.
>
>This is OK.

Hmm, already lying in bed, it occurred to me that I'm not sure about the 
exact definition for loop->exit_count. I seem to remember (I can't test 
right now) that it was set to 3 for the testcase, this would mean the main 
loop exit isn't counted here and I have to compare for >0, not >1. But I 
don't know enough about the loop detection algorithm to be 100% sure, maybe 
I have to test other loop structure members too for a correct test?

In the meantime, while browsing the code, I found another suspicious code 
fragment:

   /* Some targets (eg, C4x) need to initialize special looping
      registers.  */
#ifdef HAVE_doloop_begin
   {
     rtx init;

     init = gen_doloop_begin (counter_reg,
                              GET_CODE (iterations) == CONST_INT
                              ? iterations : const0_rtx, iterations_max,
                              GEN_INT (loop->level));
     if (init)
       {
         start_sequence ();
         emit_insn (init);
         sequence = gen_sequence ();
         end_sequence ();
         emit_insn_after (sequence, loop->start);
       }
   }
#endif

Do we need to prefer loop->top here too? The comment doesn't specify if the 
initialization is needed on every loop run...

Maybe the originator of the code should comment on the patches? Michael?

>(You should say how you tested your patch; I'm assuming you did a
>bootstrap on powerpc-linux.)

Well, since I've caught a cold my mind is a bit dizzy, so I didn't want to 
ask for commit before sleeping over it :-), but I passed my status to the 
list to get some comments.

BTW, I will try to turn both testcases in this thread into executable 
testcases.

Franz.