combine ignoring a check

Message ID 7d95d644-006e-5533-ed9a-d476124a19ac@acm.org
State New
Headers show

Commit Message

Nathan Sidwell Jan. 10, 2017, 2:12 p.m.
This patch addresses an issue Nicolai found while browsing combine.c. 
He noticed some checking code that looked like:

   for (i = 0; i < NUM_ARGS; i++)
     if (!cond)
       break;

   for (i = 0; i < NUM_ARGS; i++)
     if (!other_cond)
       break;

   if (i == NUM_ARGS)
     do_the_thing;

the intent is that both cond and other_cond are true for all args, but 
the first loop's negative result (i != NUM_ARGS) is being ignored.  This 
patch fixes that problem, and I took the opportunity to merge both loops 
into a single loop:
    ok = true;
    for (i = 0; ok && i < NUM_ARGS; i++)
      if (!cond || !other_cond)
        ok = false;
    if (ok)
      do_the_thing;

Segher commented on IRC that a single loop would be slower.  I disagree. 
  These days the optimizer is smart enough to turn that boolean logic 
into direct control flow, and a single loop is certainly smaller code (I 
checked).  The only case that might be faster would be an early out if 
the first loop resulted in a negative.  But I doubt that's significant, 
particularly as the first loop's check is the more expensive one and the 
second loops check is very cheap.

booted & tested on x86_64-linux.  Thanks to Nic for finding this problem 
and helping test it.

nathan
-- 
Nathan Sidwell

Comments

Segher Boessenkool Jan. 11, 2017, 11:11 p.m. | #1
On Tue, Jan 10, 2017 at 09:12:38AM -0500, Nathan Sidwell wrote:
> Segher commented on IRC that a single loop would be slower.  I disagree. 


Slower *and less readable*, which is the main point.  Oh well.

> -      /* Make sure this PARALLEL is not an asm.  We do not allow combining

> +	 Neither can this PARALLEL be not an asm.  We do not allow combining


Delete the first "not" here please.  Okay for trunk with that change.
Thanks for the patch!


Segher

Patch hide | download patch | download mbox

2017-01-10  Nathan Sidwell  <nathan@acm.org>
	    Nicolai Stange  <nicstange@gmail.com>

	* combine.c (try_combine): Don't ignore result of overlap checking
	loop.  Combine overlap & asm check into single loop.

Index: combine.c
===================================================================
--- combine.c	(revision 244226)
+++ combine.c	(working copy)
@@ -2785,22 +2785,24 @@  try_combine (rtx_insn *i3, rtx_insn *i2,
 	 (Besides, reload can't handle output reloads for this.)
 
 	 The problem can also happen if the dest of I3 is a memory ref,
-	 if another dest in I2 is an indirect memory ref.  */
-      for (i = 0; i < XVECLEN (p2, 0); i++)
-	if ((GET_CODE (XVECEXP (p2, 0, i)) == SET
-	     || GET_CODE (XVECEXP (p2, 0, i)) == CLOBBER)
-	    && reg_overlap_mentioned_p (SET_DEST (PATTERN (i3)),
-					SET_DEST (XVECEXP (p2, 0, i))))
-	  break;
+	 if another dest in I2 is an indirect memory ref.
 
-      /* Make sure this PARALLEL is not an asm.  We do not allow combining
+	 Neither can this PARALLEL be not an asm.  We do not allow combining
 	 that usually (see can_combine_p), so do not here either.  */
-      for (i = 0; i < XVECLEN (p2, 0); i++)
-	if (GET_CODE (XVECEXP (p2, 0, i)) == SET
-	    && GET_CODE (SET_SRC (XVECEXP (p2, 0, i))) == ASM_OPERANDS)
-	  break;
+      bool ok = true;
+      for (i = 0; ok && i < XVECLEN (p2, 0); i++)
+	{
+	  if ((GET_CODE (XVECEXP (p2, 0, i)) == SET
+	       || GET_CODE (XVECEXP (p2, 0, i)) == CLOBBER)
+	      && reg_overlap_mentioned_p (SET_DEST (PATTERN (i3)),
+					  SET_DEST (XVECEXP (p2, 0, i))))
+	    ok = false;
+	  else if (GET_CODE (XVECEXP (p2, 0, i)) == SET
+		   && GET_CODE (SET_SRC (XVECEXP (p2, 0, i))) == ASM_OPERANDS)
+	    ok = false;
+	}
 
-      if (i == XVECLEN (p2, 0))
+      if (ok)
 	for (i = 0; i < XVECLEN (p2, 0); i++)
 	  if (GET_CODE (XVECEXP (p2, 0, i)) == SET
 	      && SET_DEST (XVECEXP (p2, 0, i)) == SET_SRC (PATTERN (i3)))