Joern Rennecke <joern.renne...@embecosm.com> writes:
> Quoting Richard Sandiford <rdsandif...@googlemail.com>:
>> Joern Rennecke <joern.renne...@embecosm.com> writes:
>>> 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.
>>
>> But if you're saying that ARCompat wants different copies of the same insn
>> (with the same uid) to have different lengths, then please fix dbr_schedule
>> so that it uses different uids. The "i == 0" thing is just too hackish IMO.
>
> Implemented with the attached patch.
> I've been wondering if I should use copy_insn, but that doesn't seem to make
> any real difference after reload.
> Also, copying only PATTERN, INSN_LOCATION, and REG_NOTES into the new
> insn obtained from make_insn_raw had seemed a possibility, but there is no
> firm assurance that we will only see insn with the code INSN.
Yeah, that's unfortunate.
> In the end, continuing to use copy_rtx to make the copy seems least likely to
> break something. And now that the copying is in one place, it's easier to
> change the way it is done if you want to try that.
Agreed.
> We waste a bit of memory (temporarily, as it is garbage collected) by using
> make_insn_raw just to increment cur_insn_uid. Alternatives would be to
> move the function inside emit-rtl.c, or have emit-rtl.h provide a means to
> increment cur_insn_uid. Well, or we could directly access
> crtl->emit.x_cur_insn_uid, but that would certainly be hackish.
I agree using crtl->emit.x_cur_insn_uid from reorg.c would be hackish.
I think it would be better to put the new function in emit-rtl.c
and just have:
/* Return a copy of INSN that can be used in a SEQUENCE delay slot,
on that assumption that INSN itself remains in its original place. */
rtx
copy_delay_slot_insn (rtx insn)
{
/* Copy INSN with its rtx_code, all its notes, location etc. */
insn = copy_rtx (insn);
INSN_UID (insn) = cur_insn_uid++;
return insn;
}
OK with that change, thanks.
Richard
