[PATCH] D80240: [OPENMP50]Initial codegen for 'affinity' clauses.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Few comments, the alloca question is my only real concern. Parts of this are trivial NFC and can go in to make the patch smaller. Parts won't be needed after D80222 <https://reviews.llvm.org/D80222> landed :)



================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:2444
+    break;
+  }
   case OMPRTL__tgt_target: {
----------------
This will clash with D80222. I guess once that one is in you can just remove these parts and it will still work fine :)


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:5402
-};
-} // namespace
-
----------------
Feel free to commit the code movement parts separately as an NFC commit to make the diff cleaner.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:470
+  /// } kmp_task_affinity_info_t;
+  QualType KmpTaskAffinityInfoTy;
   /// struct kmp_dim {  // loop bounds info casted to kmp_int64
----------------
We really need to move these into OMPKinds.def as well. Not in this patch but soon.


================
Comment at: clang/test/OpenMP/task_affinity_codegen.cpp:54
+  // kmp_task_affinity_info_t affs[<num_elem>];
+  // CHECK: [[AFFS_ADDR:%.+]] = alloca %struct.kmp_task_affinity_info_t, i64 [[NUM_ELEMS]],
+  // store i64 %21, i64* %__vla_expr0, align 8
----------------
I'm not so sure about dynamic allocas (without stack save/restore). If this happens in a loop we can run into problems fast, right?


================
Comment at: clang/test/OpenMP/task_affinity_codegen.cpp:132
+
+#endif
----------------
Does it make sense to use the script to auto-generate the check lines? Makes updates way easier and cleaner. We could have the "C-code" explanation as comment above the pragma still.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80240