Quoting Richard Sandiford <rdsandif...@googlemail.com>:
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.
It's about describing complex interactions of length adjustments that derive from branch shortening and length added for (un)alignment for scheduling purposes. Expressed naively, these can lead to cycles. I could manage with the combination of length / lock_length attribute, but there was a fine line between tuning for optimal scheduling and risking cycles. So, as Richard Biener has pointed out, that design was not optimal. What is really needed is a way to express priorities between instructions when to inhibit length shrinkage from one iteration to another. Ports that have no issues with negative feedback in the length calculations don't need to worry about priorities; the length locking mechanism will be a no-op anyways, so the question when it is being applied is moot. In the case you have negative feedback, the default, to apply length locking immediatly to all instructions, gives the fastest convergence, although it will likely give a sub-optimal shortening solution. The length variation for the ARC are not alike: there are branches that are subject to branch shortening in the usual way, but they might shrink when other things shrink. When we are iterating starting at minimal length and increasing lengths, these branches should be stopped from shrinking, as they likly will grow back again in the next iteration. OTOH there are potentially short instructions that are lengthened to archive scheduling-dictated alignment or misalignment. These should be allowed to change freely back and forth. Unless we have a rare cycle that only depends on alignments. So, to get this right without having the port writer to worry too much when a length adjustment causes a cycle, we want all varying length instructions subject to length locking to break cycles, but not with the same priority. Specifying this as an iteration count is an easy way to implement this, without having to make shorten_branches identify every cycle and the participating instructions that are oscillating in length. The latter would actually run up against computability problems in distinguishing a single complex cycle and multiple independent cycles. If you like, we can describe the iter_threshold parameter in a different way, just saying that it indicates a priority, 0 being the first to get length locking applied, and the larger the number, the later.
