-
Notifications
You must be signed in to change notification settings - Fork 14
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
Auto option ranks #84
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…artup point. Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
… still not working. Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Still incomplete because of vector/matrix combination cases. Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
… deduction in binary operations Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Principle: diminish code paths at maximum by reducing the conditions checked to an absolute minimalist level. Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…ed function for each action. + prepare a fix for the <failed> type analysis of the "method call solver". This fail is due to the fact that we are past the semantic analysis, so we don't have proper scope tracking. The lookup cannot work if we don't provide a starting scope. That starting scope is reconstructed artificially using the scopes/token map collection. Unfortunately, the fast lookup is now using an intermediate map that filters by function only. If we don't include the unnamed blocks, Lookup will systematically fail for any object within curly brace or an if block, for block etc. To solve that problem, I prepared a "non disjointed" interval query system. It's an unfortunate change from Log(n) by query to O(N) by query though. Also we have a memory fest since these are node containers. We might want to consider Howard Hinnant stack allocator soon after. Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Now able to locate the symbols because the starting scope is correctly reconstructed, using the new IntervalCollection class which is able to support query for non-disjointed intervals, which is a more difficult case than what we had up to now. We still keep the previous map to functions because it's faster to query. Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…ntrinsic type. For instance "?Texture2D" as scope, and "Load" as method will end up in a "LevelUp" that isn't "/" but is "". The empty path was never never meant to be a possible output of LevelUp function, but it does happen in case of levelup from non rooted symbols. Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…gained in power. To fallback on an exhibition of the problem again it's enough to just mention the call to floor which is unregistered as long as azslc is concerned. Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
galibzon
approved these changes
Apr 11, 2023
moudgils
approved these changes
Apr 13, 2023
Signed-off-by: siliconvoodoo <vivien.oddou@siliconstudio.co.jp>
…claration of' x’changes meaning of 'x') Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is the branch to introduce static analysis for option ranks.
Here is a little history of the development of that branch.
Idea
The main concept is to start from options's seenat (references throughout the code).
From a seenat, we determine its AST node, and find out if it's connected to a subtree.
Then we walk that tree recursively to accumulate the cost of its nodes.
Each time we encounter a function call node, we lookup that function by reconstructing the scope context at that position, and using the Lookup function. We establish the cost of the function, recursively and accumulate and cache function costs to memoize. (this is no typo, it's called memoization).
Problems
binary ops
We quickly encountered a problem of limitations of the type system by testing on StandardPBR, some function's overloads couldn't be decided, because of use of binary expressions:
This exception occurs and is interceptible and thus visible when run under the debugger:
This is meant to be continuable, but will result in an internal type which can cause error #41 at times.
These are example of the contexts where StandardPBR hit such limit:
or
or
So, I implemented the binary expression solver for the type system to enrich the cost result.
Methods
Method tracing is harder than free function calls because the starting scope is harder to figure out. I had to refactor the cost analyzer structure by taking inspiration from what's happening in the seenat system. I eventually found a streamlined implementation that only takes 3 supplementary lines of code by exploiting the TypeofExpr system. It supports a.b() but also a.A::b() or a.::A::b() as well.
Scopes
I got a problem with the scope to IdentifierUID (symbol) map. Until now, the system to find out about the symbol covering a token space, was used by the --bindingdep system. It only used functions to populate that map.
The invariant was that functions never overloap, and we never have functions inside functions.
Therefore the set of intervals is disjoint.
It allowed for fast and simple queries using lower_bound (
Infimum
more exactly).In our current case, we needed full scope reconstruction including inner scopes.
Therefore, the invariant was broken and I had to extend the map with a new class, called
IntervalCollection
which is much more powerful and can now support queries for the closest scope even in case of overlaps.This is based on the mathematical idea that scopes (intervals) activated by a point are the
set intersection of the set_of_intervals_starting_earlier and set_of_intervals_ending_later
.I tried to early-optimize that function as much as possible by avoiding temporary allocations, but
set
is a node based container so allocations are legion. We may recourse to Howard Hinnant stack allocator later.That said, my benchmark found no measurable impact from that new system, this is the comparative run on StandardPBR:
we're probably good for now.
Results of new features
Between a run with only the function follower, and the full system as it is now, we have a richer and better tracking of option costs, this is the difference between the early implementation and the last one with method follow and binary expressions:
DirectionalLights is radically different notably.
This is it, the rest is in the commit messages and the code itself.
Bests.