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
Min/max/if optimizations based on the insights from rival-analyze #87
Min/max/if optimizations based on the insights from rival-analyze #87
Changes from 23 commits
ff3b2ef
9cbf9e4
33f5566
0c174db
007ce20
441caa7
c07cd6e
7037ccf
fd1275e
5ad41cd
b26b236
1445846
936c72f
5fe3f68
4b2080d
757f9be
145d410
646f266
af8950d
9a1db61
9976e64
b2af4c8
3a8b855
93ef101
d389cdd
da3942e
8d2a955
f2dd254
4a7bb19
ec2b709
0fd937c
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.
This is a no-op, right?
(or #f x)
is justx
.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.
My thinking was like this:
since we are using batches, we may have duplicates and this why I made
or
operation.The thing is that
make-hint
function first will create a mask for all the instructions,and mark
#t
nodes that are roots (they always need to be executed to write some result invreg
vector).Then it starts a traverse in the reverse order.
This line of code catches the following case:
Imagine that we have a list of expressions as:
(list '(< (cos x) 0) '(assert (< (cos x) 0)))
The algorithm first will mark roots
<
andassert
as#t
- should be executed.And then, the algorithm will start to analyze instruction by instruction.
It will look at
assert ...
and figure out thatassert
, for example, is always true - which means that(< (cos x) 0)
basically should not be executed.And here a mistake can happen,
(< (cos x) 0)
actually should be executed because it is another root and some result should be written intovregs
, if we mark(< (cos x) 0)
as#f
- there will be no output invregs
.Therefore, the algorithm before marking anything as
#f
checks whether it previously was marked as#t
and if not - it is safe to mark it as#f
, otherwise, the instruction should be executedThere 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.
Yeah I get all that. My proposal is delete this line of code entirely. If no one else wants this line of code, its hint is already set to
#f
(that's the default). If someone else wants it, good, we don't disturb it.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.
Oh, right right, here this line of code is useless
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 guess the same operation can be removed everywhere
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.
The tests represent a random testing of the hints implementation.
It samples random hyperrects in a global box of
[-100, 100]
and points inside these hyperrects.The hyperrects are analyzed using
rival-analyze
and obtained hint is executed for points.The tests verify that execution with hint and without hint produces the same results.
Additionally, the tests check the percentage of instruction executions being skipped by hint.
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.
Can you move the tests to a separate file? Otherwise, sure.
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 did not review the tests closely.
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.
Yes, sure