[PATCH] D153924: Allowing exception handling in OpenMP target regions when offloading to AMDGCN or NVPTX targets

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:432
+  "Target '%0' does not support exception handling."
+  " To allow code generation for '%0', 'catch' statement will be replaced by a no operation instruction.">;
 }
----------------
Check the style of other messages, they have a single sentence and start with a lower case letter.
Also, the explanation doesn't make sense for users. They don't know about traps, basic blocks, etc.
Maybe:
`target '%0' does not support exception handling; 'throw' is assumed to be never reached`
`target '%0' does not support exception handling; 'catch' block is ignored`
and nothing for try.
Finally, these need a group so users can also disable them.


================
Comment at: clang/lib/CodeGen/CGException.cpp:12
 //===----------------------------------------------------------------------===//
-
 #include "CGCXXABI.h"
----------------
unrelated, please add back


================
Comment at: clang/lib/CodeGen/CGException.cpp:450-451
+        << T.str();
+    EmitTrapCall(llvm::Intrinsic::trap);
+  } else if (const Expr *SubExpr = E->getSubExpr()) {
     QualType ThrowType = SubExpr->getType();
----------------
You should be able to return after the EmitTrap to simplify the code.


================
Comment at: clang/lib/CodeGen/CGException.cpp:624
+  const bool is_omp_gpu_target =
+      (CGM.getLangOpts().OpenMPIsDevice && (T.isNVPTX() || T.isAMDGCN()));
+  if (is_omp_gpu_target) {
----------------
Check the LLVM style guide, or the surrounding code, for variable naming.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3837
+    Opts.Exceptions = exceptions_user_enabled;
+    Opts.CXXExceptions = exceptions_user_enabled;
   }
----------------
I don't get this. Is this needed?


================
Comment at: clang/test/OpenMP/target_throw_message.cpp:6
+// RUN: -Xopenmp-target -fcxx-exceptions -emit-llvm -S %s -o - 2>&1 | FileCheck %s -v --check-prefix=TEST1
+// TEST1: {{Target 'amdgcn-amd-amdhsa' does not support exception handling. To allow code generation for 'amdgcn-amd-amdhsa', 'throw' expressions will be replaced by traps}}
+
----------------
It is very suspicious that you cannot use cc1 and verify. Also, various options are odd. 
I don't think we want the driver here, but we want cc1. We need to find out why this is not working.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153924