[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:661
+    AfterIP = InsertPointTy(ForkBB, ForkBB->end());
+  }
 
----------------
fghanim wrote:
> jdoerfert wrote:
> > Why do we need all of this? Can't we just *not do it* instead? This is code complexity that we should avoid.
> Depends. 
> If we want to conform with the way things are done in clang; namely, not have unreachable blocks, then yes we need to do this. If not, then no, nothing needs to change. An optimization pass will be executed at some point later that should clean all that up.
> 
> However, we should be careful, for example, The lit test for `critical` checks that no basic blocks were generated from the rest of the body that comes after the infinite loop. So if the choice is to not conform with clang, then we should keep an eye on these lit tests, and disable such checks for the OMPBuilder.
> If we want to conform with the way things are done in clang;

It's not like we introduce much extra code, break anything, or make the final result different.


>  If not, then no, nothing needs to change. An optimization pass will be executed at some point later that should clean all that up.

Let's go with that solution and keep this code here simple, less error prone, and easier to manage.


> However, we should be careful, for example, The lit test for critical checks that no basic blocks were generated from the rest of the body that comes after the infinite loop. So if the choice is to not conform with clang, then we should keep an eye on these lit tests, and disable such checks for the OMPBuilder.

We already do things different and that will only become more evident (TRegions!). For example, to simplify this code we do *not* cache runtime calls (anymore). That is we emit a new get_thread_id call every time. (We know the OpenMPOpt pass will clean it up eventually.) I get that the tests change and for a while we will have clang and OMPBuilder check lines. Though, once the clang CG is gone there is arguably no difference anymore because the OMPBuilder behavior is then the default. As soon as we have the privatization parts properly hooked up we can even start running the OMPBuilder by default and soon after removing clang CG parts. If anything, we should modernize the clang tests as they are a constant critique point that hinders outside involvement. We could start using the llvm/utils/update_XXXX_checks scripts for example. We could also minimize the check lines and focus on the most important bits only. (I prefer the update scripts with the pending extensions, e.g., D69701)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73285