[Openmp-commits] [PATCH] D90103: Add OpenMP for optimization

[Abid via Phabricator via Openmp-commits]

abidmalikwaterloo marked an inline comment as done.
abidmalikwaterloo added a comment.

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.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:198
+__OMP_RTL(__kmpc_for_static_init_4, false, Void, IdentPtr, Int32, Int32, Int32Ptr, Int32Ptr, Int32Ptr, Int32Ptr, Int32, Int32 )
+__OMP_RTL(__kmpc_for_static_fini, false, Void, IdentPtr, Int32)
 
----------------
jdoerfert wrote:
> This has not bee rebased for moths, all clang generated functions should be in here by now.
LLVM 11 OMPKinds.def has all the RTL call which makes this update redundent.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:83
+    CallInst *current_call_init_instruction = nullptr;
+  };
+
----------------
jdoerfert wrote:
> 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.
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.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:427
+      return changed;
+    }
+  
----------------
jdoerfert wrote:
> 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.
> 
Hopefully, this will be taken care of by the new patch 


================
Comment at: llvm/test/Transforms/OpenMP/parallelMergeForLoop.ll:909
+;CHECK: call void @__kmpc_barrier(
+;CHECK: ret void
----------------
jdoerfert wrote:
> I kind of doubt this test passes given that the functions are marked `optnone`. Did you run the tests and confirmed they pass?
"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. 


================
Comment at: llvm/test/Transforms/OpenMP/parallelMergeForLoop.ll:909
+;CHECK: call void @__kmpc_barrier(
+;CHECK: ret void
----------------
abidmalikwaterloo wrote:
> jdoerfert wrote:
> > I kind of doubt this test passes given that the functions are marked `optnone`. Did you run the tests and confirmed they pass?
> "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. 
I included four tests that take care of most cases except the nested control statements.


================
Comment at: llvm/test/Transforms/OpenMP/parallel_for_loop_merging.cpp:38
+// CHECK-NEXT call void @__kmpc_barrier(
+// CHECK : ret void
----------------
jdoerfert wrote:
> How can this test pass without running optimizations, not that we actually want to run a C++ test here anyway. 
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90103/new/

https://reviews.llvm.org/D90103