Sergey Fedorov <serge.f...@gmail.com> writes:
> On 20/04/16 16:33, Alex Bennée wrote:
>> Sergey Fedorov <sergey.fedo...@linaro.org> writes:
>>> diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c
>>> index 3edf6a6f971c..5c69de20bc69 100644
>>> --- a/tcg/arm/tcg-target.inc.c
>>> +++ b/tcg/arm/tcg-target.inc.c
>>> @@ -121,6 +121,13 @@ static inline void reloc_pc24(tcg_insn_unit *code_ptr,
>>> tcg_insn_unit *target)
>>> *code_ptr = (*code_ptr & ~0xffffff) | (offset & 0xffffff);
>>> }
>>>
>>> +static inline void reloc_pc24_atomic(tcg_insn_unit *code_ptr,
>>> tcg_insn_unit *target)
>>> +{
>>> + ptrdiff_t offset = (tcg_ptr_byte_diff(target, code_ptr) - 8) >>
>>> 2;
>> This seems like something a tcg_debug_assert should be ensuring we don't
>> overflow.
>
> Then we should probably put the same assert into reloc_pc24() as well.
>
>>
>>> + tcg_insn_unit insn = atomic_read(code_ptr);
>> Don't we already know what the instruction should be or could there be
>> multiple ones?
>
> Actually, it is always what tcg_out_b_noaddr() generates. I'm not sure
> it's worthwhile to introduce tcg_out_b_atomic() or something similar
> here.
No I don't think so. It depends if the branch instruction is going to
have multiple potential conditions (so requiring the read) or always be
the same.
>
>>
>>> + atomic_set(code_ptr, (insn & ~0xffffff) | (offset & 0xffffff));
>> Please use deposit32 to set the offset like the aarch64 code.
>
> Will do.
>
>>
>>> +}
>>> +
>>> static void patch_reloc(tcg_insn_unit *code_ptr, int type,
>>> intptr_t value, intptr_t addend)
>>> {
>>> @@ -1038,6 +1045,16 @@ static void tcg_out_call(TCGContext *s,
>>> tcg_insn_unit *addr)
>>> }
>>> }
>>>
>>> +void arm_tb_set_jmp_target(uintptr_t jmp_addr, uintptr_t addr)
>>> +{
>>> + tcg_insn_unit *code_ptr = (tcg_insn_unit *)jmp_addr;
>>> + tcg_insn_unit *target = (tcg_insn_unit *)addr;
>>> +
>>> + /* we could use a ldr pc, [pc, #-4] kind of branch and avoid the
>>> flush */
>> So why don't we?
>
> I think because it's a bit more expensive to take this kind of branch.
> If we assume direct jumps are taken much more times than TB patching
> then it's preferable to optimize for direct jumps instead of for
> patching.
Looking again I see the comment came from code motion so I'm not overly
fussed its just comments like that always leave me hanging. "We could
...." and I want to know why we don't then ;-)
>
> Kind regards,
> Sergey
>
>>
>>> + reloc_pc24_atomic(code_ptr, target);
>>> + flush_icache_range(jmp_addr, jmp_addr + 4);
>>> +}
>>> +
>>> static inline void tcg_out_goto_label(TCGContext *s, int cond, TCGLabel *l)
>>> {
>>> if (l->has_value) {
>>>
--
Alex Bennée
