On Wed, Oct 31, 2012 at 11:22 AM, Richard Sandiford <rdsandif...@googlemail.com> wrote: > Richard Biener <richard.guent...@gmail.com> writes: >> On Tue, Oct 30, 2012 at 5:14 PM, Joern Rennecke >> <joern.renne...@embecosm.com> wrote: >>> Quoting Richard Biener <richard.guent...@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