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. Richard
