[responding because you kept me on cc:] Joern Rennecke <joern.renne...@embecosm.com> writes: > This uses the same interface as my previous patch: > http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00473.html , > but I refined the algorithm for the get_insn_variants > mechanism to work properly with the reworked ARC port - > http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01891.html - > the only user so far, and added some documentation. > > Bootstrapped in r193731 on i686-pc-linux-gnu .
In http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00029.html the conversation was roughly: > Here's a very general (and rather complicated) interface, I wasn't after something as complicated as that. I just wanted to model alignment. and it looks like this patch implements the very general and rather complicated interface. :-) Obviously you're completely free to do that, but it means that any comments from me are going to be same (and as unhelpful and unproductive) as last time. So I still think I should bow out of this one. In case that seems unfair, here's a summary of my concerns: 1) As Richard B says, having "locked lengths" with the comment "care must be taken to avoid cycles" doesn't sound like good design. So the question was: without this, why would the length be going up and down "arbitrarily", even though we're trying to reach a fixed point? And the good answer seemed to be: because the backend wants to grow some instructions in order to give a particular (mis)alignment to later ones. There's no inherent cycle problem with that, because although the length of an instruction can decrease when the padding it previously contained is no longer needed, the instruction addresses themselves still only move in one direction. We don't need a locked length for this case. That kind of alignment optimisation sounds like a useful thing to have, but it should be modelled explicitly. The new patch does model it explicitly but still keeps the "locked length: to avoid cycles" thing as well. 2) Another reason the lengths could go up and down "arbitrarily" is because the backend wants to perform if-conversion, predication, call-it-what- you-will on the fly during shorten_branches, and can therefore delete or restore instructions relative to previous iterations. I'm still not convinced that's something we should support. 3) As mentioned previously, I don't think ADJUST_LENGTH should be aware of ADJUST_LENGTH calls for other instructions. The way that the ARC port "simulates" instructions between the insn passed in the previous and current calls doesn't seem right. 4) The patch provides a complex interface and allows for complex decisions to be made, but the results of those decisions aren't stored in the rtl. That's not a problem for ARC because ARC keeps an on-the-side FSM to track these things. I don't think we should encourage or require that though. FWIW, one interface that would deal with the alignment side of things is: unsigned HOST_WIDE_INT preferred_insn_alignment (rtx insn); which returns an instruction's preferred alignment, and which could (if useful) be called for SEQUENCEs and/or the insns inside them. rtx longer_insn_form (rtx insn, unsigned HOST_WIDE_INT current_length, unsigned HOST_WIDE_INT target_length); which returns a pattern that would turn INSN from being CURRENT_LENGTH to TARGET_LENGTH bytes in length, or null if no such pattern exists. shorten_branches would only use these hooks when optimising the insns for speed rather than size (the posted patch doesn't seem to account for that). It could record the original pattern internally so that it can revert to that pattern if the extra padding is no longer needed. longer_insn_form could be used for label alignment as well as instruction alignment. There's the potential for a third hook to return the possible target lengths for a given instruction, a bit like the one in the patch, so that alignment could be distributed over several instructions. I think that's an unnecessary complication at this stage though. (I don't think the original ARC submission supported it.) But like I say, that's all just for the record. Richard
