Skip to content
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

Fix complexity analysis #2380

Merged
merged 1 commit into from
Jan 11, 2023
Merged

Fix complexity analysis #2380

merged 1 commit into from
Jan 11, 2023

Conversation

quentin
Copy link
Member

@quentin quentin commented Dec 26, 2022

The complexity analysis was ignoring intrinsics operations that can contain functor calls. It had an impact on the reordering of conditions that would move a functor call too early when it is within an intrinsic operation.

fix #2373

@quentin quentin marked this pull request as ready for review December 26, 2022 10:02
@codecov
Copy link

codecov bot commented Dec 26, 2022

Codecov Report

Merging #2380 (5b42c1e) into master (87c7e4f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2380   +/-   ##
=======================================
  Coverage   77.53%   77.54%           
=======================================
  Files         467      467           
  Lines       30779    30785    +6     
=======================================
+ Hits        23864    23871    +7     
+ Misses       6915     6914    -1     
Impacted Files Coverage Δ
src/ast/transform/SemanticChecker.cpp 97.04% <100.00%> (-0.01%) ⬇️
src/ram/analysis/Complexity.cpp 100.00% <100.00%> (ø)
...ouffle/datastructure/ConcurrentInsertOnlyHashMap.h 87.40% <0.00%> (-0.79%) ⬇️
src/ram/utility/Visitor.h 53.78% <0.00%> (-0.76%) ⬇️
src/ast/analysis/typesystem/Type.cpp 47.43% <0.00%> (+0.59%) ⬆️

@XiaowenHu96
Copy link
Collaborator

If I understand the problem correctly, the optimizer was not considering the dependence between branch check and functor call. And the fix is to give functor call a larger complexity. If so, can we give it an INT_MAX to make it clear that we want functor calls to be put at last?

@quentin
Copy link
Member Author

quentin commented Jan 5, 2023

I agree, the current fix is not sufficient in some situations and associating the maximum complexity rank to functor call should solve the issue entirely.

@@ -62,7 +62,7 @@ int ComplexityAnalysis::getComplexity(const Node* node) const {
}

int visit_(type_identity<UserDefinedOperator>, const UserDefinedOperator&) override {
return 10;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per the comment, make it the maximum value of Int.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

The complexity analysis was ignoring intrinsics operations that can
contain functor calls. It had an impact on the reordering of conditions
that would move a functor call too early when it is within an intrinsic
operation.

fix souffle-lang#2373
Copy link
Collaborator

@XiaowenHu96 XiaowenHu96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@quentin quentin merged commit 453ae6d into souffle-lang:master Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assertion violation in ConcurrentFlyweight.h `Idx < SlotCount.load(std::memory_order_relaxed)'
2 participants