jdoerfert marked 12 inline comments as done.
jdoerfert added inline comments.
================
Comment at: clang/include/clang/Basic/OpenMPKinds.h:23-24
enum OpenMPDirectiveKind {
-#define OPENMP_DIRECTIVE(Name) \
- OMPD_##Name,
#define OPENMP_DIRECTIVE_EXT(Name, Str) \
----------------
ABataev wrote:
> Better to make in a separate NFC patch.
Agreed, will do.
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3483
+ auto *OMPRegionInfo =
dyn_cast_or_null<CGOpenMPRegionInfo>(CGF.CapturedStmtInfo);
+ if (!EmitChecks || ForceSimpleCall || !OMPRegionInfo ||
+ !OMPRegionInfo->hasCancel()) {
----------------
ABataev wrote:
> I would add an option, something like `-fopenmp-experimental` for all changes
> with OpenMPBulder unless it at least matches in functionality/performance
> with the existing solution.
We need the experimental flag eventually so I can add it here.
FWIW, there is only one functional difference for barriers and I will actually
use this as a first task of the OpenMP-aware middle-end optimization. A new
patch will be put for review shortly and once accepted we get better
`om_get_thread_num` call merging than we could have in the front-end anyway.
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3493
return;
+
// Build call __kmpc_cancel_barrier(loc, thread_id);
----------------
ABataev wrote:
> Better to remove all these formatting changes from the patch.
Sure.
================
Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:33
+ /// IDs for all OpenMP directives.
+ enum DirektiveKind {
+#define OMP_DIRECTIVE(Enum, ...) Enum,
----------------
ABataev wrote:
> 1. `DirectiveKind`
> 2. Is it possible to merge these 2 types in LLVM and in clang into 1?
1. Agreed.
2. Yes! Clang needs to eventually use the one here. I can already make that
change now or we wait a bit longer and have both coexist. The change would
probably be something along the lines of: `using OpenMPDirectiveKind =
llvm::OpenMPIRBuilder::DirectiveKind` in clang instead of the enum definition.
We probably also want to include `llvm/IR/OpenMPKinds.def` where
`OPENMP_DIRECTIVE_EXT` is now used.
================
Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:108
+ DefaultIdent->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
+ DefaultIdent->setAlignment(Align(8));
+ }
----------------
ABataev wrote:
> Is this correct for 32-bit platforms? Maybe rely on the frontend when
> creating structures and all other required feature things?
None of these properties, e.g., the alignment, should ever be wrong. It might
differ from what we have now on a certain platform but it should not be wrong
or incompatible. That being said, we can certainly think about asking the
frontend to generate global declarations for us (types should be the same). I
don't see an immediate benefit of doing so but if we have a reason it is
certainly doable.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69785/new/
https://reviews.llvm.org/D69785
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
