[Openmp-commits] [PATCH] D127399: [OpenMP] Ensure createXXX functions will always call updateToLocation

[Michael Kruse via Phabricator via Openmp-commits]

Meinersbur added a comment.

Why the pattern to just return without doing anything without an insert location? IMHO this should be an error instead of silently ignoring things.

In D127399#3583537 <https://reviews.llvm.org/D127399#3583537>, @jdoerfert wrote:

> I don't think the Builder should reset to entry. It's an implicit contract that is hard to maintain and arguably useless as there is no reason to believe one would insert two "OpenMP things" at exactly the same point. IPs during codegen generally move and we should make all IPs explicit. These guards were introduced in a single patch (IIRC) and should not serve a purpose.

I completely agree and already encountered problems with the reset location having become invalid between that guard and its dtor.



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1846-1848
+  if (!updateToLocation(Loc))
+    return nullptr;
+
----------------
The insert- and debug location is already set inside `createLoopSkeleton`, setting it here has no effect.

On line 1849 `updateToLocation` is then called and it handles the "no insert location" case by not connecting the loop skeleton to anything (as the caller wishes). `return null` makes this impossible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127399