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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

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.



================
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)
 
----------------
This has not bee rebased for moths, all clang generated functions should be in here by now.


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


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:427
+      return changed;
+    }
+  
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90103