This is an archive of the discontinued LLVM Phabricator instance.

[mlir][OpenMP] Add checks and tests for hint clause and fix empty hint
ClosedPublic

Authored by shraiysh on Apr 5 2022, 11:39 PM.

Details

Summary

This patch handles empty hint value for critical and atomic constructs.

This also adds checks and tests for hint clause on atomic constructs.

Diff Detail

Event Timeline

shraiysh created this revision.Apr 5 2022, 11:39 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
shraiysh requested review of this revision.Apr 5 2022, 11:39 PM

Another way to handle this would be to give up the string based syntax for hint and instead have an integer based syntax. i.e. hint(5) instead of hint(uncontended, nonspeculative) - with the value of hint instead of the names. Last time, there was some resistance for the integer based syntax for readability. I do not have a strong preference for one way or another. Let me know if integer based syntax sounds good.

shraiysh updated this revision to Diff 420724.Apr 6 2022, 12:11 AM

Replace hint() with hint(none)

Another way to handle this would be to give up the string based syntax for hint and instead have an integer based syntax. i.e. hint(5) instead of hint(uncontended, nonspeculative) - with the value of hint instead of the names. Last time, there was some resistance for the integer based syntax for readability. I do not have a strong preference for one way or another. Let me know if integer based syntax sounds good.

I was recommending the string-based syntax for readability. I would suggest keeping it, but if we continue to face issues then we can go to the integer syntax.

Do we need some llvm it translation tests as well?

Do we need some llvm it translation tests as well?

Hints are basically unused for translation to LLVM IR at the moment. Clang doesn't seem to use them either. So, there should be no tests for that.

shraiysh updated this revision to Diff 421421.Apr 7 2022, 11:31 PM

Added LLVM IR Translation tests for omp.critical construct. Atomic construct translation ignores the hint clause in current implementation hence no tests for the same.

NimishMishra added inline comments.Apr 10 2022, 10:04 PM
mlir/test/Dialect/OpenMP/invalid.mlir
935

One question: Will we ever reach this point from a legal fortran source code?

A capture construct proceeds as:

!$omp atomic capture
     update / capture / capture
     capture / update / write
!$omp end atomic

/ delimits the three combinations in capture construct we can have. Here, capture statement is v=x while write is x=expr. Is it possible to define a hint clause here?

shraiysh marked an inline comment as done.Apr 11 2022, 2:09 AM
shraiysh added inline comments.
mlir/test/Dialect/OpenMP/invalid.mlir
935

If the lowering is implemented correctly, it won't. However it is important to note that these verifiers try to avoid generation of malformed operations while lowering. Incorrectly implemented lowering could create an operation like this where hint was supplied to the update operation. If we assume that the lowering code will always be correct, then we don't need verifiers for any operation :) . Moreover, the checks here are independent of lowering - these are just to assert whether an input MLIR is well-formed or not and one can obviously write erroneous MLIR like this.

shraiysh marked an inline comment as done.Apr 17 2022, 9:39 PM

Gentle ping for review!

NimishMishra accepted this revision.Apr 18 2022, 12:16 AM

LGTM

mlir/test/Dialect/OpenMP/invalid.mlir
935

Thanks.

This revision is now accepted and ready to land.Apr 18 2022, 12:16 AM
peixin accepted this revision.Apr 20 2022, 5:25 AM

@kiranchandramohan Is this OK to you? The lowering atomic read/write depends on this patch, so can we give high priority for this?

@kiranchandramohan Is this OK to you? The lowering atomic read/write depends on this patch, so can we give high priority for this?

Fine with me.

shraiysh updated this revision to Diff 424072.Apr 20 2022, 6:33 PM

Thanks for the reviews. Rebase with main.

do added a subscriber: do.Apr 21 2022, 9:04 AM