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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
