On 20/04/16 17:40, Alex Bennée wrote:
> 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.
So I mean "regarding goto_tb, it's always branch immediate,
unconditional". But concerning the function name, it should only patch
the immediate offset of the instruction.
>
>>>> + 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 ;-)
So let's leave it as is?
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) {
