mem: remove duplicated trainning req of prefetcher#777
mem: remove duplicated trainning req of prefetcher#777
Conversation
when `mshrAliasFailed`/`mshrArbFailed`/`isHitInWriteBuffer`, the request will be replayed, it should not train the prefetcher. Change-Id: I45f209a8ed78d4f17850971b7af8f8b0b12395aa
📝 WalkthroughWalkthroughModified the BaseCache::recvTimingReq function to conditionally notify the Miss probe (ppMiss) only when MSHR alias did not fail, MSHR arbitration did not fail, and the request did not hit in the write buffer. This prevents miss signaling during error or edge case conditions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mem/cache/base.cc`:
- Around line 826-828: The check for replayable misses is duplicated; extract
the predicate into a single helper (e.g., add a method on BaseCache like
canReplayMiss(const Packet* pkt) or isRetryAllowed(const Packet* pkt) that
returns !pkt->mshrAliasFailed() && !pkt->mshrArbFailed() &&
!pkt->isHitInWriteBuffer()), then replace the direct predicate at the ppMiss
notify site (where ppMiss->notify(pkt) is called) and in
BaseCache::CpuSidePort::recvTimingReq with calls to that helper so both sites
share the same logic.
| if (!pkt->mshrAliasFailed() && !pkt->mshrArbFailed() && !pkt->isHitInWriteBuffer()) { | ||
| ppMiss->notify(pkt); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Miss probe declarations / notify sites ==="
rg -n -C3 '\bppMiss\b|new ProbePointArg<.*>\(.*"Miss"' src
echo
echo "=== Probe listener registration sites ==="
rg -n -C4 'regProbeListeners|ProbeListener|addEventProbe|add.*Probe' src
echo
echo "=== Miss-probe related registrations in cache + prefetch code ==="
rg -n -C4 '\bppMiss\b|"Miss"' src/mem/cache src/mem/cache/prefetchRepository: OpenXiangShan/GEM5
Length of output: 50374
🏁 Script executed:
#!/bin/bash
# Find all places where listeners are registered to the "Miss" probe
echo "=== Searching for Miss probe listener registration in prefetch code ==="
rg -n 'PrefetchListener|addListener.*Miss' src/mem/cache/prefetch/
echo
echo "=== BaseCache::regProbeListeners to see how listeners connect ==="
rg -n -A10 'BaseCache::regProbeListeners' src/mem/cache/base.cc
echo
echo "=== All listeners that might connect to cache probes ==="
rg -n 'class.*Listener.*ProbeListenerArgBase' src/mem/cache src/mem/mem_object.cc
echo
echo "=== Check if non-prefetch code uses or listens to cache Miss ==="
rg -n '"Miss"' src/mem/cache/base.cc | head -20Repository: OpenXiangShan/GEM5
Length of output: 2294
🏁 Script executed:
#!/bin/bash
# Check the retry predicate at lines 3237-3238
echo "=== Retry predicate context around 3237-3238 ==="
sed -n '3230,3250p' src/mem/cache/base.cc
echo
echo "=== Miss notify context around 826-828 ==="
sed -n '820,835p' src/mem/cache/base.cc
echo
echo "=== Verify the three packet predicates are defined ==="
rg -n 'mshrAliasFailed|mshrArbFailed|isHitInWriteBuffer' src/mem/packet.hh | head -20Repository: OpenXiangShan/GEM5
Length of output: 1734
Only prefetch training listens to the ppMiss probe, but the replay predicate is now duplicated.
Verification confirms that only the prefetcher's PrefetchListener subscribes to the cache's "Miss" probe point (see src/mem/cache/prefetch/base.cc:438), so the guard at lines 826–828 does not suppress replayed misses from non-prefetch consumers. However, the condition duplicates the retry predicate at BaseCache::CpuSidePort::recvTimingReq (lines 3237–3239)—both check !mshrAliasFailed() && !mshrArbFailed() && !isHitInWriteBuffer() (via inverted OR). If either site is modified later without updating the other, the two behaviors could diverge. Consider extracting this predicate to a helper method or constant to keep them in sync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mem/cache/base.cc` around lines 826 - 828, The check for replayable
misses is duplicated; extract the predicate into a single helper (e.g., add a
method on BaseCache like canReplayMiss(const Packet* pkt) or
isRetryAllowed(const Packet* pkt) that returns !pkt->mshrAliasFailed() &&
!pkt->mshrArbFailed() && !pkt->isHitInWriteBuffer()), then replace the direct
predicate at the ppMiss notify site (where ppMiss->notify(pkt) is called) and in
BaseCache::CpuSidePort::recvTimingReq with calls to that helper so both sites
share the same logic.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
|
🚀 Performance test triggered: spec06-0.8c |
|
[Generated by GEM5 Performance Robot] Ideal BTB PerformanceOverall Score
|
3 participants
when
mshrAliasFailed/mshrArbFailed/isHitInWriteBuffer, the request will be replayed, it should not train the prefetcher.Change-Id: I45f209a8ed78d4f17850971b7af8f8b0b12395aa
Summary by CodeRabbit