|
From: Josef W. <Jos...@gm...> - 2012-10-01 14:40:47
|
Hi Florian,
Am 30.09.2012 19:17, schrieb Florian Krohm:
> - In cachegrind: Clarify what LL cache should be used when there are
> more that 3 levels in the cache hierarchy. Update docs.
See other mail (and below).
> - Improve cannot-autodetect warnings from cachegrind. They typically
> read like so:
>
> Warning: Cannot auto-detect cache config on MIPS32, using one
> or more defaults
>
> Mentioning an architecture is redundant as we run natively. And
> "using one or more defaults" is a usability disaster. Why not say
> *what* default?
I vote for
"Cannot auto-detect cache config, using defaults (run with -v to see)"
Just looking at reduced-valg-patch...
+ // FIXME: needs clarification for num_levels > 3 see also warning below
+ // FIXME: whether it needs adjustment
+ ll = locate_cache(ci, UNIFIED_CACHE, ci->num_levels);
+
+ if (ll && ci->num_levels > 2) {
+ VG_(dmsg)("warning: L%u cache found, using its data for the "
+ "L2 simulation.\n", ci->num_levels);
+ }
I think this warning should not talk about L2, but "LL" (last level).
Do we want to add "vg_assert(ci->num_levels>1);" here?
+ */
+static Int
+Intel_cache_info_aux(Int level, cache_t* I1c, cache_t* D1c, cache_t* LLc)
...
I think this should return both L2 and L3 data now.
There is also some code reinterpreting parameters of the
micro-ops trace cache on Intel P4 for L1. Such kind of using
auto-detected values in different ways should be in the tool.
To be able to do the same now, we need another cache kind
TRACE_CACHE, and cachegrind can check for this if there is
no info for an L1 INSN_CACHE.
On the other hand, not sure it's worth it.
Josef
|
|
From: Florian K. <br...@ac...> - 2012-10-04 23:46:56
|
Hi Josef,
thanks for the review!
On 10/01/2012 10:40 AM, Josef Weidendorfer wrote:
> Hi Florian,
>
> Am 30.09.2012 19:17, schrieb Florian Krohm:
>> - In cachegrind: Clarify what LL cache should be used when there are
>> more that 3 levels in the cache hierarchy. Update docs.
>
> See other mail (and below).
Yep, noted.
>
>> - Improve cannot-autodetect warnings from cachegrind. They typically
>> read like so:
>>
>> Warning: Cannot auto-detect cache config on MIPS32, using one
>> or more defaults
>>
>> Mentioning an architecture is redundant as we run natively. And
>> "using one or more defaults" is a usability disaster. Why not say
>> *what* default?
>
> I vote for
> "Cannot auto-detect cache config, using defaults (run with -v to see)"
I like that, too.
> +
> + if (ll && ci->num_levels > 2) {
> + VG_(dmsg)("warning: L%u cache found, using its data for the "
> + "L2 simulation.\n", ci->num_levels);
> + }
>
> I think this warning should not talk about L2, but "LL" (last level).
Yes, you're right. I'll change it.
> Do we want to add "vg_assert(ci->num_levels>1);" here?
We could. But it's not needed as it would imply that we have an I/D L1
caches *and* a unified cache at level 1. That cannot be. In fact,
a comment in libvex.h says:
Additionally, if there exists
a unified cache at a particular level then no other cache exists
at that level.
> +static Int
> +Intel_cache_info_aux(Int level, cache_t* I1c, cache_t* D1c, cache_t* LLc)
> ...
>
> I think this should return both L2 and L3 data now.
>
Yes, that needs to be fixed up. I really only copied the code in place
and made minimum changes to fill in VexCacheInfo. I did not want the
patch to become too large. Also, I need to read up on this cpuid beast
first... not familiar with it.
> There is also some code reinterpreting parameters of the
> micro-ops trace cache on Intel P4 for L1. Such kind of using
> auto-detected values in different ways should be in the tool.
> To be able to do the same now, we need another cache kind
> TRACE_CACHE, and cachegrind can check for this if there is
> no info for an L1 INSN_CACHE.
> On the other hand, not sure it's worth it.
I will get back to you on this one once I understand it better.
My plan is to check in what I had posted including your suggestions.
That'll happen this weekend unless there are further comments.
Then get going on the followup patches.
Florian
|
|
From: Josef W. <Jos...@gm...> - 2012-10-05 08:51:38
|
Am 05.10.2012 01:46, schrieb Florian Krohm: >> Do we want to add "vg_assert(ci->num_levels>1);" here? > > We could. But it's not needed as it would imply that we have an I/D L1 > caches *and* a unified cache at level 1. That cannot be. You are right. If the auto-detected cache only has 1 level, one has to specify parameters anyway. Thanks, Josef |