Re: RFA: hookize ADJUST_INSN_LENGTH

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Oct 31, 2012 at 11:22 AM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Richard Biener <richard.guenther@gmail.com> writes:
>> On Tue, Oct 30, 2012 at 5:14 PM, Joern Rennecke
>> <joern.rennecke@embecosm.com> wrote:
>>> Quoting Richard Biener <richard.guenther@gmail.com>:
>>>
>>>> Apart from the iteration_threshold the hookization would be
>>>> straight-forward.
>>>> Now I cannot decipher from the patch what functional change it introduces
>>>> ;)
>>>
>>>
>>> The only change occurs if we reach an iteration count of MAX_INT iterations
>>> -
>>> which should already be indicative of a problem.
>>>
>>> At the MAX_INTth iteration, we will apply the length locking logic to
>>> instructions inside a delay slot sequence as well.
>>>
>>> If we disregard this exceptional case, there should be no functional changes
>>> unless someone defines TARGET_ADJUST_INSN_LENGTH.
>>>
>>> uid_lock_length gets initialized to allocated with XCNEWVEC.  So, in
>>> particular, the elements corresponding to instructions inside delay slot
>>> sequences are initialized to zero.
>>>
>>> As default hook sets *iter_threshold to MAX_INT when inside a delay
>>> sequence,
>>> and doesn't change length, the max operation with uid_lock_length is a
>>> no-op,
>>> and *niter < iter_threshold is true, hence a length change results in
>>> updating the length immediately, without changing uid_lock_length.
>>>
>>> In the case that we are not inside a delay slot sequence, the default hook
>>> leaves iter_threshold as 0, and applies ADJUST_INSN_LENGTH.  Thus, when
>>> niter
>>> is 0 or larger, as is the case for the ordinary looping operation, we
>>> always find *niter >= iter_threshold, and thus apply the length locking
>>> mechanism.
>>>
>>> If we are in the preliminary pass, or doing the single !increasing
>>> iteration,
>>> niter is set to -1, so *inter < iter_threshold is always true, so again we
>>> update the length immediately.
>>
>> Ok, I see.
>>
>> The patch is ok then.
>
> I should probably have piped up earlier, but I'm really not sure about it.
> ADJUST_INSN_LENGTH as defined now is telling us a property of the target.
> The patch instead ties the new hook directly into the shorten_branches
> algorithm, which I think is a bad idea.
>
> IMO, the length attributes and ADJUST_INSN_LENGTH should be treated as
> a fused process: every value returned by a length attribute (whether min,
> default or current) should be filtered through ADJUST_INSN_LENGTH before
> being used.  Whether ADJUST_INSN_LENGTH decreases or increases the original
> length doesn't matter, because only the output of ADJUST_INSN_LENGTH should
> be used.
>
> Joern's patch makes us use ADJUST_INSN_LENGTH for delay slot insns,
> which I agree is a good thing in itself.  It's the all-important
> iteration parameter that feels wrong.
>
> TBH, I still don't understand what property of the target we're trying
> to describe here.  I gather it has something to do with alignment.
> But I'd really like a description of that target property, independent
> of the shorten_branches implementation.

Yeah, the iteration parameter doesn't feel right, I agree.  But
ADJUST_INSN_LENGTH
is only used in this code.

Which is why I originally said if the patch would not add the
iteration parameter
it would be obvious ...

So, consider my approval revoked.

Richard.

> Richard