Re: Ping: RFA: add lock_length attribute to break branch-shortening cycles

[Richard Sandiford <rdsandiford at googlemail dot com>]

Joern Rennecke <joern.rennecke@embecosm.com> writes:
> Quoting Richard Sandiford <rdsandiford@googlemail.com>:
>> The fact that we even have shared unique ids is pretty bad -- and surely
>> a contradiction in terms -- but I think both ways of handling them rely
>> on the length being the same for all copies.  If we don't record a length
>> (your version), we won't set something_changed if the length of this copy
>> did in fact change, which could lead to branches remaining too short.
>
> That is not the case, as the current length of the inner insn is added to
> new_length (of the outer insn).
>
>> If we do record a length (current version),
>
> In the current version, the length of the inner insn is calculated anew
> each iteration, so only the out-of-sequence copy suffers from the bogosity.
>
>> we could end up changing
>> the length of previous copies that have already been processed in
>> this iteration.
>
> Both that, and also using the length or the previous insn for the inner
> length calculation, and ratcheting them up together.
> In fact, this can make delay slot instructions ineligible for the delay slot
> they are in.  Also, the unwanted length cross-interference can violate
> alignment invariants, and this leads to invalid code on ARCompact.

But if you're saying that ARCompat wants different copies of the same insn
(with the same uid) to have different lengths, then please fix dbr_schedule
so that it uses different uids.  The "i == 0" thing is just too hackish IMO.

>> is simpler and means we get the natural behaviour if anyone ever does
>> "fix" dbr_schedule.
>>
>> I think first_pass should be !optimize.
>
> That would not work for CASE_VECTOR_SHORTEN_MODE, where it controls setting
> the mode of BODY.
>
> And as we have to use the first_pass variable there, I thought it's simpler
> to also use it in these other places; if we ever want to switch the
> decision on when to use increasing and when to use decreasing lengths,
> the algorithm continues to work this way, without the need to replace the
> optimize test in all places with the new test.

I don't mind having a local boolean to control whether we're applying
increasing or decreasing lengths, if you prefer that to plain "optimize".
I just don't like the condition changing from the second approximation
to the third.

>>> @@ -1382,7 +1399,8 @@ shorten_branches (rtx first ATTRIBUTE_UN
>>>  	  insn_current_address += (new_length - tmp_length);
>>>  #endif
>>>
>>> -	  if (new_length != insn_lengths[uid])
>>> +	  if (new_length != insn_lengths[uid]
>>> +	      && (new_length > insn_lengths[uid] || first_pass))
>>>  	    {
>>>  	      insn_lengths[uid] = new_length;
>>>  	      something_changed = 1;
>>
>> Same first_pass comment here.  I.e.:
>>
>> 	  if (optimize
>> 	      ? new_length > insn_lengths[uid]
>> 	      : new_length < insn_lengths[uid])
>
> It's about testing a condition first that can eliminate further tests.
> if the length is unchanged - which would be expected to be the case
> for most variable length instructions after the first iteration -
> we don't need to test the other conditions.
> And optimize, being a global variable, is relatively expensive on
> some host platforms.
> It could get even more expensive if/when we ever go multithreaded.
>
>> I think you need an "else" that does:
>>
>>     insn_current_address += insn_lengths[uid] - new_length;
>
> When the condition is not fulfilled, we want to keep the length from the
> previous iteration.

Right, that's what I mean.  So we need to make sure that the difference
between the address of the current instruction and the address of the
next instruction also doesn't change.  The code as posted was:

	  else
	    {
	      new_length = insn_current_length (insn);
	      insn_current_address += new_length;
	    }

#ifdef ADJUST_INSN_LENGTH
	  /* If needed, do any adjustment.  */
	  tmp_length = new_length;
	  ADJUST_INSN_LENGTH (insn, new_length);
	  insn_current_address += (new_length - tmp_length);
#endif

	  if (new_length != insn_lengths[uid]
	      && (new_length > insn_lengths[uid] || first_pass))
	    {
	      insn_lengths[uid] = new_length;
	      something_changed = 1;
	    }

which always leaves insn_current_address based off new_length,
even if new_length is out of kilter with insn_lengths[uid].
It looked like it should be:

	  else
	    {
	      new_length = insn_current_length (insn);
	      insn_current_address += new_length;
	    }

#ifdef ADJUST_INSN_LENGTH
	  /* If needed, do any adjustment.  */
	  tmp_length = new_length;
	  ADJUST_INSN_LENGTH (insn, new_length);
	  insn_current_address += (new_length - tmp_length);
#endif

	  if (...honour new_length...)
	    {
	      insn_lengths[uid] = new_length;
	      something_changed = 1;
	    }
	  else
	    insn_current_address += insn_lengths[uid] - new_length;

so that insn_current_address continues to match the final value
of insn_lengths[uid].

Richard