Re: [PATCH 2/2] perf arm-spe: Parse more SPE fields and store source

[German Gomez <german.gomez@arm.com>]

Hi Leo,

Thanks for taking the time, and sorry for the late reply

On 27/02/2022 13:20, Leo Yan wrote:
> On Mon, Feb 21, 2022 at 08:41:43PM +0000, German Gomez wrote:
>
> [...]
>
>> Some comments:
>>
>> # ARM_SPE_OP_ATOMIC
>>
>> This might be a hack, but can we not represent it as both LD&SR as the
>> atomic op would combine both?
>>
>> data_src.mem_op = PERF_MEM_OP_LOAD | PERF_MEM_OP_STORE;
> BTH, I don't understand well for this question, but let me explain a
> bit:
>
> We cannot use 'LOAD | STORE' to present the atomic operation.  Please
> see Armv8 ARM section D10.2.7 Operation Type packet, it would give out
> more details.  Atomic operation is an extra attribution for a load or
> store operations, it could be an atomic load or store, or
> load-acquire/store-release instructions, or
> load-exclusive/store-exclusive instructions.

I will check, thanks.

My thinking was that atomics perform some load-modify-store operation
hence why I suggested combining LOAD&STORE flags. But I admit didn't try
running the instructions myself so I didn't check the actual records.

> [...]
>
>> # ARM_SPE_OP_SVE_SG
>>
>> (I'm sorry if this is too far out of scope of the original patch. Let
>> me know if you would prefer to discuss it on a separate channel)
>>
>> On a separate note, I'm also looking at incorporating some of the SVE
>> bits in the perf samples.
>>
>> For this, do you think it makes sense to have two mem_* categories in
>> perf_mem_data_src:
>>
>> mem_vector (2 bits)
>> - simd
>> - other (SVE in arm64)
> I think we can define below vector types:
>
> PERF_MEM_VECTOR_SIMD
> PERF_MEM_VECTOR_SVE
>
> The tricky thing is "other"... Based on the description for "Operation
> Type packet payload (Other)" in the Armv8 Arm, I think we even need to
> add an extra operation type PERF_MEM_OP_OTHER and assign it to
> data_src.mem_op field.
>
>> mem_src (1 bit)
>> - sparse (scatter/gather loads/stores in SVE, as well as simd)
> How about the naming "mem_attr" for new field and define two
> attributions:
>
> PERF_MEM_ATTR_SPARSE  -> Gather/Scatter operation
> PERF_MEM_ATTR_PRED    -> Predicated operation
>
> Just remind, we cannot only approve within Arm related developers,
> it's good to seek more wider review from other Arch developers when
> you send new patch set.

Agree. On second thought, the mention of sve seems very arch-specific
for this...

Recently the idea of adding arch-specific flags to the branch entries
was mentioned in [1]. Perhaps we could suggest something similar for
this. Or leave simd/sve as a perf-tool-only feature for now.

[1] https://lore.kernel.org/all/Ygv4cmO%2Fzb3qO48q@robh.at.kernel.org/

>
> Thanks,
> Leo