Quoting Richard Sandiford <rdsandif...@googlemail.com>:
The fact that we even have shared unique ids is pretty bad -- and surely
a contradiction in terms -- but I think both ways of handling them rely
on the length being the same for all copies. If we don't record a length
(your version), we won't set something_changed if the length of this copy
did in fact change, which could lead to branches remaining too short.
That is not the case, as the current length of the inner insn is added to
new_length (of the outer insn).
If we do record a length (current version),
In the current version, the length of the inner insn is calculated anew
each iteration, so only the out-of-sequence copy suffers from the bogosity.
we could end up changing
the length of previous copies that have already been processed in
this iteration.
Both that, and also using the length or the previous insn for the inner
length calculation, and ratcheting them up together.
In fact, this can make delay slot instructions ineligible for the delay slot
they are in. Also, the unwanted length cross-interference can violate
alignment invariants, and this leads to invalid code on ARCompact.
Both would be wrong, but at least the current version
is simpler and means we get the natural behaviour if anyone ever does
"fix" dbr_schedule.
I think first_pass should be !optimize.
That would not work for CASE_VECTOR_SHORTEN_MODE, where it controls setting
the mode of BODY.
And as we have to use the first_pass variable there, I thought it's simpler
to also use it in these other places; if we ever want to switch the
decision on when to use increasing and when to use decreasing lengths,
the algorithm continues to work this way, without the need to replace the
optimize test in all places with the new test.
@@ -1382,7 +1399,8 @@ shorten_branches (rtx first ATTRIBUTE_UN
insn_current_address += (new_length - tmp_length);
#endif
- if (new_length != insn_lengths[uid])
+ if (new_length != insn_lengths[uid]
+ && (new_length > insn_lengths[uid] || first_pass))
{
insn_lengths[uid] = new_length;
something_changed = 1;
Same first_pass comment here. I.e.:
if (optimize
? new_length > insn_lengths[uid]
: new_length < insn_lengths[uid])
It's about testing a condition first that can eliminate further tests.
if the length is unchanged - which would be expected to be the case
for most variable length instructions after the first iteration -
we don't need to test the other conditions.
And optimize, being a global variable, is relatively expensive on
some host platforms.
It could get even more expensive if/when we ever go multithreaded.
I think you need an "else" that does:
insn_current_address += insn_lengths[uid] - new_length;
When the condition is not fulfilled, we want to keep the length from the
previous iteration. We could put
"new_length = insn_length[uid];" in the else clause, but that'd be pointless,
as new_length is dead at that point.
