This is an archive of the discontinued LLVM Phabricator instance.

[mlir][OpenMP] OpenMP Synchronization Hints stored as IntegerAttr
ClosedPublic

Authored by shraiysh on Oct 7 2021, 2:57 PM.

Details

Summary

hint-expression is an IntegerAttr, because it can be a combination of multiple values from the enum omp_sync_hint_t (Section 2.17.12 of OpenMP 5.0)

Diff Detail

Event Timeline

shraiysh created this revision.Oct 7 2021, 2:57 PM
shraiysh requested review of this revision.Oct 7 2021, 2:57 PM
shraiysh retitled this revision from OpenMP Synchronization Hints stored as IntegerAttr to [mlir][OpenMP] OpenMP Synchronization Hints stored as IntegerAttr.Oct 7 2021, 2:58 PM
shraiysh updated this revision to Diff 378023.Oct 7 2021, 3:02 PM

Removed minor unrelated test-case change.

Thanks @shraiysh for the fix.

Would it be possible to keep the new representation but custom print or parse the hint as strings (since strings are more readable)?

It can be done and it certainly would be more readable, but one catch is that the frontend has to first convert an integer into such strings, which we will then again convert to the integer. Plus we would lose the assemblyFormat. Basically, the lowering would be something like this

#pragma omp critical hint(10) // C/C++ or Fortran
omp.critical hint(speculative) hint(contended) // MLIR
__kmpc_omp_critical_with_hints(..., 10) // LLVM IR

Please let me know if I should change it to this.

It can be done and it certainly would be more readable, but one catch is that the frontend has to first convert an integer into such strings, which we will then again convert to the integer. Plus we would lose the assemblyFormat. Basically, the lowering would be something like this

#pragma omp critical hint(10) // C/C++ or Fortran
omp.critical hint(speculative) hint(contended) // MLIR
__kmpc_omp_critical_with_hints(..., 10) // LLVM IR

Please let me know if I should change it to this.

The builder (which is an interface for a frontend) can take in a hint as an integer, the parser and printer only has to deal with conversions.

For the assembly format you need a custom part only for the hint.

The format can also be hint(a,b).

Okay, that sounds good. I will do that.

shraiysh updated this revision to Diff 378100.Oct 7 2021, 11:09 PM

Added custom printer and parser for hints as strings.

ftynse requested changes to this revision.Oct 8 2021, 12:25 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
378–379

Having an extra unit attr just for the sake of pretty-printing sounds obnoxious. We can just skip printing the hint altogether if it has the default (0) value. The only thing it will require is to have custom<SynchronizationHint>($hint) also print (or not) the "hint" keyword.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
974

Please document top-level functions.

990

This will just ignore keywords that are not in this list, I would rather emit an error than silently discard input.

999

hint(none) works by accident because unknown keywords are ignored, hint(all_possible_hints) has exactly the same effect...

This revision now requires changes to proceed.Oct 8 2021, 12:25 AM
shraiysh updated this revision to Diff 378132.Oct 8 2021, 2:09 AM

Addressed comments.

shraiysh marked 4 inline comments as done.Oct 8 2021, 2:10 AM
ftynse accepted this revision.Oct 8 2021, 6:14 AM
ftynse added inline comments.
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
1014

Nit: /// here and below for top-level comments.

This revision is now accepted and ready to land.Oct 8 2021, 6:14 AM
shraiysh updated this revision to Diff 378204.Oct 8 2021, 7:20 AM

Addressed comments. I do not have commit access, can someone please land this revision. Thank you.

LGTM.

I will merge this soon. Will you be cherry-picking this patch to fir-dev?

Will you be cherry-picking this patch to fir-dev?

I am sorry for the delay, I missed this notification somehow, just saw it. I will cherry-pick this patch to fir-dev soon.