This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Use compound operators for reduction combiner if available.
ClosedPublic

Authored by mikerice on May 5 2021, 1:44 PM.

Details

Summary

The OpenMP spec seems to require the compound operators be used for
+, *, &, |, and ^ reduction. So use these if a class has those operators.
If not try the simple operators as we did previously to limit the impact
to existing code.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=48584

Diff Detail

Event Timeline

mikerice created this revision.May 5 2021, 1:44 PM
mikerice requested review of this revision.May 5 2021, 1:44 PM
ABataev added inline comments.May 5 2021, 1:49 PM
clang/lib/Sema/SemaOpenMP.cpp
16858

Why only for records?

clang/test/OpenMP/reduction_compound_op.cpp
2

Use a script to generate checks

mikerice added inline comments.May 5 2021, 2:15 PM
clang/lib/Sema/SemaOpenMP.cpp
16858

For scalars there is no difference in functionality afaik but it does end up generating different llvm IR. The resulting IR changes in some non-trivial ways in some cases changing atomic instructions to critical sections in some of the lit tests. I didn't want to change and possibly destabilize known working codegen for no known benefit.

clang/test/OpenMP/reduction_compound_op.cpp
2

Sorry I am not familiar with such a script. Can you give me a pointer or explain?

ABataev added inline comments.May 5 2021, 2:23 PM
clang/test/OpenMP/reduction_compound_op.cpp
2

The script is llvm/utils/update_cc_test_checks.py. You can check test/OpenMP/nvptx_target_codegen.cpp as an example for args

mikerice updated this revision to Diff 343214.May 5 2021, 3:42 PM
mikerice marked 2 inline comments as done.

Used script for the test. Fixed clang-format issue.

ABataev added inline comments.May 6 2021, 4:27 AM
clang/test/OpenMP/reduction_compound_op.cpp
9–10

Add checks for -fopenmp-simd flags, that no _kmpc|tgt calls are emitted

mikerice added inline comments.May 6 2021, 2:13 PM
clang/test/OpenMP/reduction_compound_op.cpp
9–10

Thinking about this more I am not modifying any codegen so I think it's probably better to test this in the AST. The scripted codegen test makes it very difficult to tell what is being tested. Is it okay if I drop this codegen test and write an AST test instead?

ABataev added inline comments.May 6 2021, 2:43 PM
clang/test/OpenMP/reduction_compound_op.cpp
9–10

Better to keep it, if possible. Just the test for simd must very simole,withou check l8nes,just need to check that no kmpc|tgt calls are generated.

mikerice updated this revision to Diff 343542.May 6 2021, 5:26 PM

Added -fopenmp-simd check to reduction_compound_op.cpp
Regenerated checks for nvptx_target_parallel_reduction_codegen_tbaa_PR46146.cpp since it uses complex reduction with operator+=.

Ping. Anything else needed on this one?

ABataev accepted this revision.May 11 2021, 11:15 AM

LG, sorry, just missed the last update

This revision is now accepted and ready to land.May 11 2021, 11:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2021, 11:42 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript