Re: sibcall fixup

[kenner at vlsi1 dot ultra dot nyu dot edu (Richard Kenner)]

    The size is reduced, but the structure is more complicated and so in
    the end it's harder to understand, despite being able to see more of
    it at once.

I'd argue the structure is simpler to understand.

    If the conditions are complex, and "do something" is simple,
    then it is easier to analyze that way.

    Consider a hypothetical reader who is trying to figure out
    whether this code does the right thing.  If the reader sees

	if (condition1)
	  action1;

    then they immediately know that when condition1 holds, this code will
    perform action1.  They can immediately verify whether that is the
    correct action to perform in that case.
    Then, when they see

	if (condition2)
	  action1;
    
    they can do the same.

But that's what comments are for.

    If, on the other hand, the reader sees

	if (condition1
	    || condition2
	    ...

    then they can't immediately see what action will be taken when the
    first part of the condition is true, so they can't immediately
    verify that that sub-case is doing the right thing.  They need
    to scroll down to the end of the condition

Perhaps, but if the conditions are all related, as in this case, I think
it's clearer to do it in one statement, especially if it's well documented.

Here's the code in question again:

	  /* If we haven't failed yet, check if this (or safe things) ends our
	     block.  */
	  if ((sibcall || tailrecursion)
	      /* If the call was the end of the block, then we're OK.  */
	      && (end == (temp = insn)
		  /* Skip over copying from the call's return value pseudo into
		     this function's hard return register and if that's the end
		     of the block, we're OK.  */
		  || (identify_call_return_value (PATTERN (insn), &hardret,
						  &softret)
		      && end == (temp = skip_copy_to_return_value (insn,
								   hardret,
								   softret)))
		  /* Skip any stack adjustment.  */
		  || end == (temp = skip_stack_adjustment (temp))
		  /* Skip over a CLOBBER of the return value as a hard reg.  */
		  || end == (temp = skip_use_of_return_value (temp, CLOBBER))
		  /* Skip over a USE of the return value (as a hard reg).  */
		  || end == (temp = skip_use_of_return_value (temp, USE))
		  /* Skip over the JUMP_INSN at the end of the block.  */
		  || end == (temp = skip_jump_insn (temp))))
	    ;

The comment at the front says what's going on: we're testing to see if INSN
ends the basic block or is followed by things that are "safe".

    Furthermore, IIRC your diff replaced code of the form

    	/* comment here */
    	var = foo();
	if (var == ...)
		do something;

	/* comment here */
	var = bar();
	if (var == ...)
		do same thing;

    with code of the form

	/* comment here */
	if ((var = foo()) == ...
	    /* comment here */
	    || (var = bar()) = ...

    This is worse for two reasons, one being that it combines side effects
    with the conditions,

The side effect is really part of the test here and the variable being set
is only used in the condition as part of the test, so it's not a "side effect"
in the normal sense of the word.  Indeed, I think that putting it in the test
nmakes it *clear* that these assignments are only part of the test and used
for nothing else.