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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:71
+    /// Keeps map of __kmpc_static_init4 and its __kmpc_static_fini calls for each OpenMP for loop
+    std::map<CallInst *, CallInst *> call_init_fini_mapping;
+    std::map<CallInst *, BasicBlock*> call_basicblock_mapping;
----------------
sstefan1 wrote:
> abidmalikwaterloo wrote:
> > sstefan1 wrote:
> > > Maybe consider using LLVM's ADTs.? Same goes for the rest of the patch.
> > > 
> > > Also, you should follow the [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | coding style  ]]
> > Can you elaborate? std::map is llvm ADT.
> > https://llvm.org/docs/ProgrammersManual.html#map
> SmallVector, DenseMap for example, see https://llvm.org/docs/ProgrammersManual.html#dss-densemap
`std::` is from the C++ standard library. The programmers manual specifies the pros and cons and usually ADT implementations are preferable.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90103