On 06/10/2015 10:06 AM, Daniel Axtens wrote:
>
>> +void update_branch_entry(struct cpu_hw_events *cpuhw,
>> + int index, u64 from, u64 to, int pred)
>> +{
>> + cpuhw->bhrb_entries[index].from = from;
>> + cpuhw->bhrb_entries[index].to = to;
>> + cpuhw->bhrb_entries[index].mispred = pred;
>> + cpuhw->bhrb_entries[index].predicted = ~pred;
>> +}
>
> I realise you're copying existing code, but:
> - could you please rename pred? If we assign .mispred to pred
> and .predicted to ~pred, we should pick a different name for pred.
Agreed.
> - I'm really uncomfortable with the bitwise inverting a signed integer.
> Can you explain what is going on here? Looking at
> include/uapi/linux/perf_event.h, this seems to be a single bit flag:
> shouldn't this then be a logical flip rather than a bitwise one?
> (Furthermore, looking at that header, why is pred an int at all? Why not
> a bool?)
Agreed.
>
>> +
>> /* Processing BHRB entries */
>> static void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
>> {
>> - u64 val;
>> - u64 addr;
>> + u64 val, addr, tmp;
> Please don't use 'tmp' here. As far as I can tell, you use this variable
> to compute the 'to' address. The name should reflect that.
Agreed but then it will be a new preparatory patch at the beginning
of this patch series.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
