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

Add Worklist implementations #393

Merged
merged 7 commits into from
Feb 15, 2024
Merged

Add Worklist implementations #393

merged 7 commits into from
Feb 15, 2024

Conversation

haved
Copy link
Collaborator

@haved haved commented Feb 12, 2024

To avoid one big PR, here is the set of worklist implementations I have, used by the new Andersen solver.

I decided to use tightly packed vectors instead of HashMaps / HashSets. This can of course be benchmarked later.

Copy link
Owner

@phate phate left a comment

Choose a reason for hiding this comment

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

I got interrupted and could not finish my review. I will need to have another look at it. Just leaving what I have so far.

I am a little bit worried about the template parameter only being of integer type as this would limit the usefulness severely.

jlm/llvm/opt/alias-analyses/Worklist.hpp Outdated Show resolved Hide resolved
jlm/llvm/opt/alias-analyses/Worklist.hpp Outdated Show resolved Hide resolved
jlm/llvm/opt/alias-analyses/Worklist.hpp Outdated Show resolved Hide resolved
jlm/llvm/opt/alias-analyses/Worklist.hpp Outdated Show resolved Hide resolved
jlm/llvm/opt/alias-analyses/Worklist.hpp Outdated Show resolved Hide resolved
jlm/llvm/opt/alias-analyses/Worklist.hpp Outdated Show resolved Hide resolved
jlm/llvm/opt/alias-analyses/Worklist.hpp Outdated Show resolved Hide resolved
jlm/llvm/opt/alias-analyses/Worklist.hpp Outdated Show resolved Hide resolved
@haved
Copy link
Collaborator Author

haved commented Feb 12, 2024

I implemented your suggested changes, and fixed the tests (always test with --enable-asserts...).

While I think performance might be better with std::vectors, it does make the interface less general, so I moved it to util::HashSet and std::unordered_map for now. The worklist is a tight part of the algorithm, so it will be interesting to see in the future if it has an effect.

Copy link
Owner

@phate phate left a comment

Choose a reason for hiding this comment

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

The main issue I have is that this should live in libutil and not in the alias-analysis subdirectory. This is quite useful for other parts in the compiler.

I agree with you that using std::vector would be more performant due to caching effects, but that should be possible by using template specialization:

template<>
LifeWorklist<size_t> : public Worklist<size_t>
{
...
}

(written by heart, syntax might be off)
where you provide another implementation with std::vector. In other words, we can have the cake and also eat it. The cost is another implementation (and tests).

jlm/llvm/opt/alias-analyses/Worklist.hpp Outdated Show resolved Hide resolved
jlm/llvm/opt/alias-analyses/Worklist.hpp Outdated Show resolved Hide resolved
jlm/llvm/opt/alias-analyses/Worklist.hpp Outdated Show resolved Hide resolved
@haved
Copy link
Collaborator Author

haved commented Feb 13, 2024

I moved it to util, and started on specialization for integer types, but ended up with SFINAE fluff that didn't look that nice by the time it worked, so I'm trying to make it a bit more sane first

@haved
Copy link
Collaborator Author

haved commented Feb 15, 2024

I ended up not adding specializations for bounded integer typed Worklists now, as it always became quite ugly. The best solution I found was to create a class with the same interface as util::HashSet<T>, but enforce T to be integer and the bounds to be provided in the ctor. The same would have to be done for util::HashMap<K,V>, but we currently don't even have our own util::HashMap<K,V>, and it felt a bit overkill to add that now.

Also, I didn't opt to use std::move(), as this class really shouldn't be used unless std::is_trivially_copyable_v<T> either way.

Instead, I will investigate if preallocated sequential vectors makes a difference once the Worklist actually is in use.

@haved haved requested a review from phate February 15, 2024 12:38
@phate phate merged commit 2159a8c into master Feb 15, 2024
10 checks passed
@phate phate deleted the worklist-implementations branch February 15, 2024 19:21
phate added a commit that referenced this pull request Feb 15, 2024
…Constraints (#395)

This comes on top of #393. Ignore those files for now.

`Andersen::Statistics` has been modified to use the new system. A lot of
the RVSDG-related labels are shared with Steensgaard, so I can move
those strings to `Labels` if desired.

The `PassConfiguration` is what allows the choice of multiple solver
implementation, and eventually specifying what kinds of offline
preprocessing should be done.

I renamed the two horribly named constraints to `LoadConstraint` and
`StoreConstraint`, since that is the only situation they are used, and
it aligns better with the literature.

---------

Co-authored-by: Nico Reissmann <[email protected]>
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.

2 participants