[PATCH] D72400: [MLIR] Add OpenMP dialect with barrier operation

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:19
+
+static DialectRegistration<OpenMPDialect> openmpDialect;
+
----------------
DavidTruby wrote:
> rriddle wrote:
> > jdoerfert wrote:
> > > rriddle wrote:
> > > > nit: openMPDialect?
> > > If we spell out OpenMP it should be `OpenMP`.
> > MLIR uses `camelCase` variable names.
> I've changed this to ompDialect to be consistent with the namespace name change. Let me know if there's a preferred name for this though.
> MLIR uses camelCase variable names.

While that is interesting on its own, my comment was not targeted at variable naming conventions but rather towards how we commonly, and even in MLIR, treat "names". "OpenMP" is (used as) a proper name, doing anything to it seems problematic, e.g., we cannot properly search for it. We also have ample of precedence, e.g. GPU, LLVM, SPIRV, .... at least I haven't seen gPU, lLVM, or sPIRV.

> I've changed this to ompDialect to be consistent with the namespace name change. Let me know if there's a preferred name for this though.

That sounds fine to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72400