PseudoLRU non-power-of-2 #2493

ingallsj · 2020-05-28T01:19:34Z

Related issue:
Type of change: performance enhancement
Impact: API addition (no impact on existing code)
Development Phase: implementation

Release Notes

Formerly, the PseudoLRU algorithm would not replace all entries in a non-power-of-2 ways cache (or TLB). For example, PLRU with 6 ways results in way 5 (the sixth way) never being replaced unless it is otherwise invalidated. However, the way index is always "in bounds", so this is purely a performance bug, not a functional bug.
The new PseudoLRU algorithm is more verbose Chisel code, with extra if-statements to recursively descend an un-balanced binary tree.
I wrote tests to exercise the old and the new PLRU behavior. @hcook do I need to do anything else to include these in CI regressions?
While I was at it, I upgraded Replacement.scala to Chisel3.
I also added TrueLRU as an option for a replacement policy, based on code we had outside Rocket-chip.
Lastly, I exposed some more internals of the ReplacementPolicy classes as public function defs to use outside Rocket-chip.

hcook

I think if you actually want them to run on every PR CI you have to add them to UnittestSuite list in regression/Makefile

PLRU. get_next_state made my eyes cross so i applaud you for adding a unit test

BrianGraysonSiV · 2020-05-28T16:26:56Z

regression/Makefile

@@ -61,7 +61,7 @@ endif

 ifeq ($(SUITE),UnittestSuite)
 PROJECT=freechips.rocketchip.unittest
-CONFIGS=$(PROJECT).AMBAUnitTestConfig $(PROJECT).TLSimpleUnitTestConfig $(PROJECT).TLWidthUnitTestConfig
+CONFIGS=$(PROJECT).AMBAUnitTestConfig $(PROJECT).TLSimpleUnitTestConfig $(PROJECT).TLWidthUnitTestConfig $(PROJECT).ScatterGatherTestConfig $(PROJECT).PLRUUnitTestConfig $(PROJECT).PowerQueueTestConfig


This is probably too long of a line for lint et al, right?

not sure about lint in makefiles, but the line keeps getting shorter as I remove tests that I tried to add but fail in CI regressions! 🙃

BrianGraysonSiV · 2020-05-28T16:31:20Z

src/main/scala/util/Replacement.scala

+                state(this_ways-3,half_ways-1)),
+            Mux(way(log2Ceil(this_ways)-1),
+                state(half_ways-2,0),
+                get_next_state(state(half_ways-2,0), way(log2Ceil(half_ways)-1,0), half_ways)))


@johningalls-sifive can you add some comments here or in another file that explains in detail what is going on? The software-engineer in me sees all these magic -3, -2, -1 values, and the lack of comments, and although I trust you that it does the right thing, I don't know if anyone besides you can maintain this function as it currently is! :)

I agree completely and went through the PseudoLRU code again. Let me know if it's improved now!

BrianGraysonSiV · 2020-05-28T16:34:31Z

src/main/scala/util/Replacement.scala

+    }
+  }
+
+  def get_next_state(state: UInt, way: UInt, this_ways: Int): UInt = {


I'm a fan of descriptive parameter and variable names. Would either n_ways or this_nways be better than just "this_ways", which (to me at least) does not clearly convey that it is the total number of ways?

Similarly, I'd change "way" to "touched_way", and if typing that is too annoying, say val w = touched_way, and use w in the code below for conciseness. That way, people fully understand the parameters to get_next_state() even without any comments.

I changed it to tree_nways and touch_way

ingallsj · 2020-05-28T21:00:58Z

I think if you actually want them to run on every PR CI you have to add them to UnittestSuite list in regression/Makefile

this (and other) unit test that I tried to add to the PR CI regression Makefile fails with

/home/runner/work/rocket-chip/rocket-chip/src/main/resources/csrc/verilator.h:29:1: fatal error: /home/runner/work/rocket-chip/rocket-chip/emulator/generated-src/freechips.rocketchip.unittest.<name>Config.plusArgs: No such file or directory
 #endif
 ^
compilation terminated.

ingallsj · 2020-05-28T21:27:17Z

the PLRU unit test is passing running this command locally:

make -C vsim run PROJECT=freechips.rocketchip.unittest CONFIG=freechips.rocketchip.unittest.PLRUUnitTestConfig

* PLRUTest passing 2,4 ways * PLRUTest passing 3 ways * PLRUTest unexpected discrepancies with 6 ways: never replaces way 5 * PLRUTest failing 6 ways: never replaces way 5 * PLRU for non-power-of-two entries * TrueLRU * expose some class ReplacementPolicy public def * PLRU gracefully handle 1 way * add all remaining UnitTestConfig to regression * TLXbarUnitTest fails to elaborate * ECCUnitTest passes locally but fails in CI * ScatterGatherTest passes locally but fails in CI * PLRUUnitTest passes locally but fails in CI * PowerQueueTestConfig passes locally but fails in CI * more explanation in variable names and comments * PseudoLRU get_next_state touch_way_sized padTo/extract * deprecate old methods

ingallsj requested review from aswaterman and jsmithsf May 28, 2020 01:19

ingallsj added the proposal label May 28, 2020

ingallsj changed the title ~~Plru pow2~~ PseudoLRU non-power-of-2 May 28, 2020

hcook approved these changes May 28, 2020

View reviewed changes

ingallsj force-pushed the plruPow2 branch from 0289bb4 to afcc8cf Compare May 28, 2020 04:32

BrianGraysonSiV reviewed May 28, 2020

View reviewed changes

ingallsj added 16 commits May 28, 2020 15:26

PLRUTest passing 2,4 ways

2d47d33

PLRUTest passing 3 ways

908e64e

PLRUTest unexpected discrepancies with 6 ways: never replaces way 5

22103a4

PLRUTest failing 6 ways: never replaces way 5

700f08c

PLRU for non-power-of-two entries

33ce252

TrueLRU

8954035

expose some class ReplacementPolicy public def

393b418

PLRU gracefully handle 1 way

47a4171

add all remaining UnitTestConfig to regression

b04ff3a

TLXbarUnitTest fails to elaborate

b75922d

ECCUnitTest passes locally but fails in CI

56f04d7

ScatterGatherTest passes locally but fails in CI

630e11b

PLRUUnitTest passes locally but fails in CI

7fe5fd2

PowerQueueTestConfig passes locally but fails in CI

f240cd7

more explanation in variable names and comments

26bf6bc

PseudoLRU get_next_state touch_way_sized padTo/extract

5d11803

ingallsj force-pushed the plruPow2 branch from 11695b7 to 5d11803 Compare May 28, 2020 22:26

deprecate old methods

9a32c8d

ingallsj merged commit 5b8b0f7 into master May 29, 2020

ingallsj deleted the plruPow2 branch May 29, 2020 17:09

ingallsj mentioned this pull request Jun 1, 2020

PseudoLRU power-of-2-plus-1 #2498

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PseudoLRU non-power-of-2 #2493

PseudoLRU non-power-of-2 #2493

ingallsj commented May 28, 2020 •

edited

Loading

hcook left a comment

BrianGraysonSiV May 28, 2020

ingallsj May 28, 2020

BrianGraysonSiV May 28, 2020

ingallsj May 28, 2020

BrianGraysonSiV May 28, 2020

ingallsj May 28, 2020

ingallsj commented May 28, 2020

ingallsj commented May 28, 2020

PseudoLRU non-power-of-2 #2493

PseudoLRU non-power-of-2 #2493

Conversation

ingallsj commented May 28, 2020 • edited Loading

hcook left a comment

Choose a reason for hiding this comment

BrianGraysonSiV May 28, 2020

Choose a reason for hiding this comment

ingallsj May 28, 2020

Choose a reason for hiding this comment

BrianGraysonSiV May 28, 2020

Choose a reason for hiding this comment

ingallsj May 28, 2020

Choose a reason for hiding this comment

BrianGraysonSiV May 28, 2020

Choose a reason for hiding this comment

ingallsj May 28, 2020

Choose a reason for hiding this comment

ingallsj commented May 28, 2020

ingallsj commented May 28, 2020

ingallsj commented May 28, 2020 •

edited

Loading