-
Notifications
You must be signed in to change notification settings - Fork 140
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
EF SemiRing in Problem #722
Conversation
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.
LGTM overall
Did you run a test benchmark how this impact the analysis time?
#include "phasar/DataFlow/IfdsIde/EdgeFunction.h" | ||
|
||
namespace psr { | ||
template <typename AnalysisDomainTy> class SemiRing { |
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.
Hmm, as the IDESolver is already specialized on the AnalysisDomainTy
, could it make sense to implement the SemiRing
structure as a trait and not on the ide/ifds problem itself.
Question: Is the implementation of combine/extend usually/always problem specific or is it in general specific for a given AnalysisDomainTy
.
Why do I ask? Well, one would model the interface around a trait, the IDESolver would get around the virtual function call to extend/combine.
Example:
...
template<>
class SemiRing<MyAnalysisDomain> {
static EdgeFunction<l_t> extend(const EdgeFunction<l_t> &L,
const EdgeFunction<l_t> &R) {
...
}
}
class IDESolver {
void foo() {
...
auto res = SemiRing<AnalysisDomainTy>::extend(L, R);
...
}
};
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 haven't benchmarked the new implementation, but I like your idea with separating the semi ring from the problem.
However, we cannot implement the trait functions as static
as the main reason for this PR is to make the integration of edge-function caching easier. Still, we can get rid of the additional virtual call
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.
There is one problem, though: The edge-function factories within the IDE problem need the edge-function cache, as well as the extend
and combine
functions need it.
So, it is actually easier to have these both functions as members within the IDE problem; or do you have an idea, how to workaround this?
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.
Why can the function not be static for the cache? The cache state could be part of the IDE solver and in case of a miss call the static impl. but in the end, solve it as you like it best^^
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 edge functions should be specific to the problem. Two problems might accidentially have the same domain, but want to handle edge functions differently.
The idea of having the cache in the solver sounds interesting, but probably too much for this PR.
static EdgeFunction<l_t> | ||
compose(EdgeFunctionRef<IIAAAddLabelsEF> /*This*/, | ||
const EdgeFunction<l_t> & /*SecondFunction*/) { | ||
llvm::report_fatal_error("Implemented in 'extend'"); |
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 this left for legacy reasons?
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.
Currently, the EdgeFunction
interface requires compose
and join
to be implemented. The default implementation of the semi-ring methods also assumes these functions to be implemented. We may want to make them optional at some point
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.
If they not strictly required, disabling them would be a option, otherwise this could crash at some point.
Small update: I have run the IIA on some smaller coreutils and at least there I could not detect any performance impact |
#include "phasar/DataFlow/IfdsIde/EdgeFunction.h" | ||
|
||
namespace psr { | ||
template <typename AnalysisDomainTy> class SemiRing { |
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 edge functions should be specific to the problem. Two problems might accidentially have the same domain, but want to handle edge functions differently.
The idea of having the cache in the solver sounds interesting, but probably too much for this PR.
Edge functions model a bounded idempotent semi-ring.
Thereby, the extend-operation is represented as
composeWith
and the combine-operation asjoinWith
.The fixed interface of these functions makes handling dependent resources hard, especially caching edge functions.
This PR creates the option to implement
extend
andcombine
on theIDETabulationProblem
instead, making it easy to access members of the problem instance.The PR also applies this concept to the inst-interation analysis, to show how EF caching can look like.