Messages in this thread | | | Date | Mon, 10 Feb 2020 13:46:53 +0800 | From | Leo Yan <> | Subject | Re: [PATCH v4 1/5] perf cs-etm: Refactor instruction size handling |
| |
Hi Mike,
On Thu, Feb 06, 2020 at 12:36:43PM +0000, Mike Leach wrote: > Hi Leo, > > On Mon, 3 Feb 2020 at 02:07, Leo Yan <leo.yan@linaro.org> wrote: > > > > cs-etm.c has several functions which need to know instruction size > > based on address, e.g. cs_etm__instr_addr() and cs_etm__copy_insn() > > two functions both calculate the instruction size separately with its > > duplicated code. Furthermore, adding new features later which might > > require to calculate instruction size as well. > > > > For this reason, this patch refactors the code to introduce a new > > function cs_etm__instr_size(), this function is central place to > > calculate the instruction size based on ISA type and instruction > > address. > > > > For a neat implementation, cs_etm__instr_addr() will always execute the > > loop without checking ISA type, this allows cs_etm__instr_size() and > > cs_etm__instr_addr() have no any duplicate code with each other and both > > functions are independent and can be changed separately without breaking > > anything. As a side effect, cs_etm__instr_addr() will do a few more > > iterations for A32/A64 instructions, this would be fine if consider perf > > is a tool running in the user space. > > > > I prefer to take the optimisation win where I can - I always do in the > trace decoder when counting instructions over a range. > Consider that you can be processing MB of trace data, and most likely > that will be A64/A32 on a lot of the current and future platforms. > > Therefore I would keep the useful cs_etm__instr_size() function, but > also keep a single ISA check in cs_etm__instr_addr() to do > the (addr + offset * 4) calculation for non T32.
Understand. Will refine the code by following this suggestion.
Thanks, Leo Yan
| |