This is an archive of the discontinued LLVM Phabricator instance.

Add OpenMP for optimization
Needs ReviewPublic

Authored by abidmalikwaterloo on Oct 24 2020, 12:44 PM.

Details

Summary

Adding test cases

Update OpenMPOpt.cpp

Make cahnges in OpenMPOpt.cpp

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 24 2020, 12:44 PM
abidmalikwaterloo requested review of this revision.Oct 24 2020, 12:44 PM

Adding test cases

There are still no test cases.

Adding test cases

There are still no test cases.

Yes. I have tests. I will add it to the LIT and will update the patch.

reverse ping. Also, I left comments in the original review that have not been addressed.

Adding test cases

There are still no test cases.

Yes. I have tests. I will add it to the LIT and will update the patch.

Previous patch:

https://reviews.llvm.org/D75384

On the first look, you should at least clang-format this as it is not very readable right now.

There are still no tests.

.arcconfig
4

This seems unrelated.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
70

Maybe consider using LLVM's ADTs.? Same goes for the rest of the patch.

Also, you should follow the coding style

411

Seems unused.

abidmalikwaterloo marked an inline comment as done.
  • make changes in OpenMPOpt
abidmalikwaterloo marked an inline comment as done.Nov 22 2020, 9:05 AM
This comment was removed by abidmalikwaterloo.
.arcconfig
4

Should I then include the following?

"repository.callsign" : "G",
"conduit_uri" : "https://reviews.llvm.org/"

The tests should be in llvm-project/llvm/test/Transforms/OpenMP.

The tests should be in llvm-project/llvm/test/Transforms/OpenMP.

Yes, no need for new folder.

.arcconfig
4

I'm saying I don't see the reason for this to be in the patch.

abidmalikwaterloo marked an inline comment as done.Nov 23 2020, 2:59 AM
abidmalikwaterloo added inline comments.
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
70

Can you elaborate? std::map is llvm ADT.
https://llvm.org/docs/ProgrammersManual.html#map

sstefan1 added inline comments.Nov 23 2020, 3:42 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
70

SmallVector, DenseMap for example, see https://llvm.org/docs/ProgrammersManual.html#dss-densemap

abidmalikwaterloo marked an inline comment as done.
  • added test case for parallel openmp for loop merging

added a test. Would like to have feedback to add more.

Added a test for OpenMP for loop fusion. Is the sample test acceptable? I can add more in the same format.

  • Added a test case for OMP loop for merging
jdoerfert added inline comments.Jan 14 2021, 8:23 PM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
70

std:: is from the C++ standard library. The programmers manual specifies the pros and cons and usually ADT implementations are preferable.

llvm/test/Transforms/OpenMP/parallelMergeForLoop.ll
909 ↗(On Diff #313643)

I kind of doubt this test passes given that the functions are marked optnone. Did you run the tests and confirmed they pass?

llvm/test/Transforms/OpenMP/parallel_for_loop_merging.cpp
38 ↗(On Diff #313643)

How can this test pass without running optimizations, not that we actually want to run a C++ test here anyway.

The optimization code is lacking in various regards. Let's focus on addressing the test remarks first in order to upstream those. We can revisit how to proceed with the optimization code after, e.g., revisit the algorithm from scratch.

llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
198

This has not bee rebased for moths, all clang generated functions should be in here by now.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
82

LLVM style, e.g., naming conventions, still not adapted.
Various members are unsued, others seem to track information already present in the IR/CFG unmodifed.

407

This code is basically impossible to review.

  • The coding style is not LLVMs and the code is not properly formatted.
  • There is no comment explaining what is going on and why.
  • The functions have generic names unrelated to the specific functionality.
  • The number of data structures, e.g., maps and vectors, seems excessive. While their usage remains unclear it seems the approach so far "analyzes" the code first and builds up a record before taking action. It is often preferable to pick a runtime call, determine if it can be removed, do it if possible, and go to the next. Furthermore, mappings that are already available elsewhere should not be build.
abidmalikwaterloo marked an inline comment as done.Feb 4 2021, 4:32 PM

I have a new patch that I wrote with LLVM-11 .0. The current patchwork is based on LLVM-9.0. The OpenMPOpt.CPP has a lot of modifications. What is the best way to submit the new patch? Any suggestion would be helpful.

llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
198

LLVM 11 OMPKinds.def has all the RTL call which makes this update redundent.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
82

In the new patch which is based on LLVM 11 update, I took care of this for simple cases that do not include a control statement. The new patch is very small as compare to this one and used LLVM data structures for MAP and Vectors.

407

Hopefully, this will be taken care of by the new patch

llvm/test/Transforms/OpenMP/parallelMergeForLoop.ll
909 ↗(On Diff #313643)

"optnone" is part of the comments when I compile the test without any optimizations. My understanding is that the IR should be without any optimization. The c code tests are running perfectly and I am getting IR what it should be. However, when I used the IR as input some parts of the tests especially related to condition are breaking. I compacted the new patch which is small ( about 150 lines) and in line with the LLVM software requirement. I am testing it and will upload it after cleaning it.

909 ↗(On Diff #313643)

I included four tests that take care of most cases except the nested control statements.

llvm/test/Transforms/OpenMP/parallel_for_loop_merging.cpp
38 ↗(On Diff #313643)

Yes.. optimization should be there. It is a redundant test as the test should be in IR format. I will remove it when I upload the new work. However, this test work with the patch.

abidmalikwaterloo marked an inline comment as done.Feb 4 2021, 5:17 PM

The optimization code is lacking in various regards. Let's focus on addressing the test remarks first in order to upstream those. We can revisit how to proceed with the optimization code after, e.g., revisit the algorithm from scratch.

I would like to submit the tests along with the new patch. Some tests are failing with this patch. Or should I just submit all possible tests first ( whether the patch successfully runs or not)?

  1. rewrote the patch and elimate lot of redundent stuff
    • Added a test case for OMP loop for merging

uploaded a new patch which is short and works on the cases without conditional statements

Should I upload all potential tests in C/C++ format to define the scope of this patch? We can convert it to IR format later for upstreaming.

Let's focus on the tests without the optimization code.

On that front:

  1. Avoid unnecessary complexity, i.a., print, and go for simple identifiable calls, like body1()
  2. Remove everything from the test that is not needed, e.g. unrelated functions, attributes, etc.
  3. Preprocess the tests with common passes such as mem2reg, instcombine, and simplifycfg. We only want to run openmpopt on them later, never O3.

Let's focus on the tests without the optimization code.

On that front:

  1. Avoid unnecessary complexity, i.a., print, and go for simple identifiable calls, like body1()
  2. Remove everything from the test that is not needed, e.g. unrelated functions, attributes, etc.
  3. Preprocess the tests with common passes such as mem2reg, instcombine, and simplifycfg. We only want to run openmpopt on them later, never O3.

Is there a set of other common passes that are required for this level of testing? Could not figure out from the other OpenMP IR test cases.

  • A modified parallel_omp_for_loop_merge1.ll is added . Taking care of the comments
jdoerfert added inline comments.Mar 2 2021, 8:02 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
138

Why would we unpack the arguments if we can look them up in a call?

237

Let's create a revision just with the test first. The code above will require rewriting and reviewing is hard, partially because of formatting and naming conventions. FWIW, OpenMPOpt is clang-formatted.

llvm/test/Transforms/OpenMP/parallel_omp_for_loop_merge1.ll
3

can you run the update_test_checks.py script on this to create the run lines.

  • Apply the update_test_checks.py

x - Clean the code

  • clean the code
abidmalikwaterloo marked an inline comment as done.Mar 12 2021, 7:06 AM
  • Apply the update_test_checks.py

I have cleaned it

llvm/test/Transforms/OpenMP/parallel_omp_for_loop_merge1.ll
3

can you run the update_test_checks.py script on this to create the run lines.

Done