Re: ifcvt/crossjump patch: Fix PR 42496, 21803

[Bernd Schmidt <bernds at codesourcery dot com>]

On 04/10/2010 12:35 PM, Eric Botcazou wrote:
> Could you split it up though, i.e. have a first patch to fix PR 21803/42496 
> and a second patch for the additional optimization?  This helps reghunting.

Ok.  Here's the first part.  Thanks for the review.

> Adding EH checking code for calls to old_insns_match_p is a step forward but 
> isn't sufficient when -fnon-call-exceptions is enabled (Java, Ada).

The move_across_if changes (motivated by failures) use
control_flow_insn_p which I believe takes care of the issue for that
part.  Do you feel anything else is necessary in the context of this patch?

> The ATTRIBUTE_UNUSED on 'mode' in flow_find_cross_jump is already bogus, so 
> the new one in flow_find_head_matching_sequence is as well.

Eliminated the new one.

> There is no comment in flow_find_head_matching_sequence about the purpose of 
> the EH edges checking code.

It's basically a sanity check; I'm not even sure it can trigger.  I
looked at outgoing_edges_match to find things I may need to test for;
this was one the pieces I added.  I've added a small comment now; let me
know if you feel something else is necessary.

> The "Don't begin a cross-jump with a NOTE insn" block is almost identical to 
> that of flow_find_cross_jump, please factor them out.

Done.

[convert while into for]

Done.

> Why returning FALSE and not cancelling the sequence merging only?

Doesn't really matter as cond_exec_process_insns will then fail for the
same reason later on.

> +  /* Merge the blocks!  If we had matching sequences, make sure to delete one
> +     copy at the appropriate location first.  */
> 
> I'd expand on the last sentence: [...]

Done.

> There are trailing spaces and tabs on some lines in the ifcvt.c hunk.

Hopefully fixed.

Retested this version with arm-linux-gnueabi
(qemu-system-armv7{arch=armv7-a/thumb,thumb,}).  No new failures (one
set of tests now times out on a different multilib, compared to a clean
baseline).  Ok?


Bernd

Attachment: ifcvt-v8.diff
Description: Text document