[PATCH] D70712: [WIP][NFC] Adding Flush support in OpenMPIRBuilder

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Minor comments and overall fine approach. Could you update the test so I can accept it? Later I will assume changes like this are made before it is commited.



================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3879
+    OMPBuilder->emitFlush({CGF.Builder.saveIP()});
+  else {
+    // Build call void __kmpc_flush(ident_t *loc)
----------------
You can even move the code to the beginning of the function (see my comment below as well).

Nit: I personally prefer conditionals to have braces in all parts or no braces at all. I get itchy when we have a branch with and one without.


================
Comment at: llvm/include/llvm/Transforms/Utils/OpenMPIRBuilder.h:81
+  /// \param Loc The location at which the request originated and is fulfilled.
+  void emitFlush(const LocationDescription &Loc);
+
----------------
I think we settled on `CreateXXX` as naming scheme to match the `IRBuilder` for now. 


================
Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:250
+  assert(Loc.IP.getBlock() && "No insertion point provided!");
+  Builder.restoreIP(Loc.IP);
+
----------------
Can you adopt the scheme below (w/o assert for now)
```
 if (!updateToLocation(Loc))
    return;
```
I'm not certain why we support an "invalid" insertion point but the update method also sets the debug information properly.


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

https://reviews.llvm.org/D70712