Page MenuHomePhabricator

[MLIR][OpenMP] Add support for critical construct
ClosedPublic

Authored by kiranchandramohan on Jul 30 2021, 12:28 AM.

Details

Summary

This patch adds the critical construct to the OpenMP dialect. The
implementation models the definition in 2.17.1 of the OpenMP 5 standard.
A name and hint can be specified. The name is a global entity or has
external linkage, it is modelled as a FlatSymbolRefAttr. Hint is
modelled as an integer enum attribute.

Diff Detail

Event Timeline

kiranchandramohan requested review of this revision.Jul 30 2021, 12:28 AM
ftynse added inline comments.Aug 2 2021, 3:09 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
373

structure?

378

Ultra-nit: please align this line vertically with something.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
243

Nit: isn't there a getValueOr() ?

mlir/test/Dialect/OpenMP/invalid.mlir
301

Do we really want printed names this long? Can't we just have nonspeculative here?

Address review comments. Changes include,
-> Using shorter names for hint clause values.
-> Formatting and corrections to description.
-> Using getValueOr in place of hasValue, getValue etc.

kiranchandramohan marked 2 inline comments as done.Aug 2 2021, 4:23 PM
kiranchandramohan added inline comments.
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
243

Yes. Thanks for pointing this out. Fixed.

mlir/test/Dialect/OpenMP/invalid.mlir
301

I have switched to using nonspeculative instead of omp_sync_hint_nonspeculative here. I agree that from a pure readability and mlir perspective it is good to use short names.

Two reasons for using the long name in the first version,

  1. Using the name in the standard might help avoid a conversion while lowering from another intermediate representation (like parse-tree in Flang).
  2. Sometimes the clause values can clause clashes with reserved keywords in C++. For eg. for the default clause there were issues with using the private value. https://bugs.llvm.org/show_bug.cgi?id=47225

Using long names, can help avoid such issues.

ftynse accepted this revision.Aug 3 2021, 12:43 AM
This revision is now accepted and ready to land.Aug 3 2021, 12:43 AM