Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tage #443
base: dev
Are you sure you want to change the base?
Tage #443
Changes from 71 commits
ff665cf
e81673f
40e7709
6fa281d
31c871e
110c1c6
4b3617c
a55e292
17c9baf
508a2f4
e0f8121
af8d1a0
f9089e0
3e5b507
0445478
f49e538
e688a05
1f925ea
52f9688
6a286d3
d0cc56a
1c1b6ce
1b800c1
3aa7ca0
e525016
673fe87
ace2d59
416cc20
92e67a8
f051277
2a957cb
0980811
e5f52eb
37a1c34
e297e68
6dfa36c
4155ffc
601178b
5031d15
4ad630c
d2a651b
111a48f
c661294
e8683c1
00d198a
9f144e1
6f73f4e
03d807d
9afa83c
79a5c7f
e11742d
729de42
922ba4b
dd3053a
4e1717d
bff5fa8
22756e6
eba5447
768db53
14789a6
7dcbb16
c6dc5b5
f9da602
0ecdd6b
57f3575
674672f
800ce6f
b23429f
ac0d2cf
f9ecd29
f11661d
e00ec65
d87cc5d
44182e0
fdfc963
c22b9d4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the "tage" in the latter sentence meant to be that or rather "tag"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"vector" should be "array"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need the conditional statement? After doing the AND you could shift right by 63 to get your 0 or 1. Would be slightly fewer cycles and more understandable/readable in my eyes (you may disagree)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the conditional is needed here. Whats being loaded into the uint64 depends on where it is in the vector. All but the least-significant uint64s get the MSB of the next uint64 added as the LSB. But the least-significant uint64 gets isTaken added as the LSB. However, if I'm misunderstanding your Q LMK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we assert position being < size_ as above, or are there cases where this could "validly" be greater? For instance, if you are trying to update an entry that has been lost from the history because there have been too many branches in the meantime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly as you say, I don't think that this should be an assert as the core may validly try to update a history that is no longer being tracked. The reason that we should allow this is to allow the pipeline not to need to know the size of the branch history. We're already ensuring that this doesn't cause problems with our if statement on 82.