Add (port) performance sensitive randomization #17
Conversation
Co-authored-by: James Foster <jafosterja@gmail.com>
connorkuehl
requested review from
jcantrell,
Nixoncole,
tim-pugh,
nforbus and
jeffreytakahashi
connorkuehl
changed the title
There was a problem hiding this comment.
Need to pull in branch and give it a run. Besides that the code is easy to read and understand. Great job @connorkuehl @Jafosterja .
| return true; | ||
| } | ||
|
|
||
| SmallVector<Decl *, 64> randomize(SmallVector<Decl *, 64> fields) { |
There was a problem hiding this comment.
Do we need randomize() as well as Bucket::randomize() ? randomize() looks unused
There was a problem hiding this comment.
I think the requirements document specifies having a basic randomize (this one) and a cache line sensitive one (perfrandomize).
If we keep both, I'd imagine we will need to introduce a switch or something so that users can specify which one they want. In that case, this code will not be unused.
If we opt to only have the performance-sensitive randomization, then we will delete this routine, yes.
There was a problem hiding this comment.
Just for my own curiosity, this is just a stylistic question, why not pass Bucket.fields to randomize(), instead of duplicating randomize() inside the Bucket class? Is that just in case we need to tweak Bucket.randomize() in the future?
There was a problem hiding this comment.
Is that just in case we need to tweak Bucket.randomize() in the future?
Yeah, but there's a big chance we won't have to. What you described is probably better at the moment.
|
|
||
| bool BitfieldRun::isBitfieldRun() const { | ||
| // Yes. | ||
| return true; |
There was a problem hiding this comment.
Is this the commenting that we want? While literal and correct, it's not helpful.
This is a port of our Clang plugin code for the reorganization of fields.
Closes #10
Please review this so that we can ultimately diminish how hacky it feels.