Page MenuHomePhabricator

Add OpenMP for optimization
Needs ReviewPublic

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

Details

Reviewers
jdoerfert
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
71

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

Also, you should follow the coding style

431

Seems unused.

abidmalikwaterloo marked an inline comment as done.
  • make changes in OpenMPOpt
abidmalikwaterloo marked an inline comment as done.Sun, Nov 22, 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.Mon, Nov 23, 2:59 AM
abidmalikwaterloo added inline comments.
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
71

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

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

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