Skip to content

Conversation

@pollo-fritto
Copy link

The following is a brief per file description of the changes:

  • BackEnd.cpp: the changes enable the SLP vectorizer and to run optimization passes leveraging the target machine capabilities. These changes match the vectorized code produced by opt

  • Conversion.cpp: I still need to figure out if these changes are beneficial, I will benchmark them. If they are detrimental, I will remove them from the merge

  • DictIntBenchBenchmark.cpp: just an increase to the dictionary benchmark range

  • merge_benchs.sh: this script is meant to group benchmarks from different files in order to have a single plot of the same benchmark in different runs. It is a work in progress, ignore it for now.

  • SoA_dictionary.rl: This is a structure of array version of the dictionary container. It allows vectorization of get(), remove() and contains(). Still to be benchmarked, I expect a performance gain for large dictionaries

  • dictionary.rl: edit a wrong function comment

pollo-fritto and others added 18 commits August 24, 2025 11:50
…th an example on both rl and c language, with their IR outputs and yaml pass remarks
… and specification of a Target Machine for the Pass Manager Builder. This makes rlc -O2 vectorization equals to opt -O2 for rlc/tool/rlc/test/examples/vectorization/dict.rl
No more get target machine procedure on runOptimizer() but passed as a func arg
…ench script and dict test

Soa_dictionary is a structure of array version of the dictionary implementation, in this way  it possible to achieve vectorization. Now contains() get vectorized
The new merge_bench separate every different measurement in different files. In this way we can compare the same benchmark from different commits
Fix _grow() bug in size 0 case, contains() improved with early exit if first element of stride empty
contains() and _insert() were bugged, the accumulator of condition was in or instead of and, _insert() had a totally wrong implementation of distance check.
get() and remove() vectorized, remove() to be finished.
To check if the 4 above get vectorized.
…() vectorized

Update merge_bench.sh description
Soa_dictionary seems to work fine if  using _index_binary_search(), but it does not vectorize get() and remove(). Add print_dict() for testing and many print for debugging purposes
Add different approaches to index_of, none works
Copy link
Contributor

Choose a reason for hiding this comment

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

i guess this is the file with the optimized dictionary implementation? if it is better than the previous one it should just entirelly replace that

mlir::StringAttr noaliasAttr = rewriter.getStringAttr("llvm.noalias");

for (size_t i = 1; i < newF.getFunctionBody().getNumArguments(); i++){
newF.setArgAttr(
Copy link
Contributor

Choose a reason for hiding this comment

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

this we cannot add to the final commit because it is not always true that they never alias

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I will delete them. If these attributes will lead to better benchmarks, I will just write it into the report

passManager.addPass(
MPM.addPass(
createModuleToFunctionPassAdaptor(std::move(functionPassManager)));
if (emitSanitizerInstrumentation and not targetIsWindows)
Copy link
Contributor

Choose a reason for hiding this comment

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

there is something going on here that happen probably due to the rebasing. it seem that ode that was there before has been accidentally removed.

Copy link
Author

Choose a reason for hiding this comment

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

These instructions were repeated in both branches' end, I just moved them down

};
};

struct mlir::rlc::TargetInfoImpl
Copy link
Contributor

Choose a reason for hiding this comment

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

i know that this is probably due to the rebase, but does this piece of code really need to be moved away from its original location?

Copy link
Author

Choose a reason for hiding this comment

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

I need it to be before runOptimizer() because one of its arguments has this structure type

@@ -0,0 +1,126 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

is this file only needed when measuring performance from two different patches, for the purpose of comparing them?

Copy link
Author

Choose a reason for hiding this comment

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

Yes it is, if you do not need it in the repo, I will remove it

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