This is an archive of the discontinued LLVM Phabricator instance.

[Clang][OpenMP] Add the codegen support for `atomic compare`
ClosedPublic

Authored by tianshilei1992 on Jan 31 2022, 10:28 AM.

Details

Summary

This patch adds the codegen support for atomic compare in clang.

Diff Detail

Event Timeline

tianshilei1992 requested review of this revision.Jan 31 2022, 10:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2022, 10:28 AM

rebase and finish the functionality

Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2022, 11:41 AM

remove unrelated changes

jdoerfert added inline comments.Feb 2 2022, 1:34 PM
clang/lib/CodeGen/CGStmtOpenMP.cpp
6029

leftover

clang/test/OpenMP/atomic_ast_print.cpp
1 ↗(On Diff #405393)

this is a parser/sema test, not codegen. Add to the other patch.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
1360 ↗(On Diff #405379)

move into the other patch

rebase and make preparation for codegen in clang

tianshilei1992 marked an inline comment as done.

remove leftovers

tianshilei1992 edited the summary of this revision. (Show Details)Feb 6 2022, 6:02 PM
tianshilei1992 added inline comments.Feb 6 2022, 6:08 PM
clang/test/OpenMP/atomic_compare_codegen.cpp
2

The test case itself was generated from a Python script.

4

This test cannot be done because clang crashed. I filed a bug https://github.com/llvm/llvm-project/issues/53601.

7

CodeGen for -fopen-simd is wrong. It doesn't emit corresponding atomic operations. I'll fix it.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
3496 ↗(On Diff #406310)

This change will be separated.

Actually I got a question about whether we want support equality comparison for floating point values. In the patch, we don't support '<' or '>' for floating point values because we don't have corresponding atomic instruction for them. For '==', we cast them to integers and then compare the two integers. However, We all know it doesn't make too much sense to compare two floating point variables, such as x == d. The comparison of floating point variables should always do like x - d < epsilon. However, like we said before, that comparison cannot be done because of lack of instruction. So I'm not sure if we want to support floating point variables at all.

fix clang crash and rebase

tianshilei1992 marked an inline comment as done.Feb 7 2022, 11:31 AM

rebase and remove all SIMD-ONLY0 check lines as they are useless

tianshilei1992 retitled this revision from [WIP][Clang]OpenMP] Add the codegen support for `atomic compare` to [Clang]OpenMP] Add the codegen support for `atomic compare`.Feb 7 2022, 1:57 PM
tianshilei1992 added a reviewer: ABataev.
tianshilei1992 marked an inline comment as not done.
tianshilei1992 added inline comments.Feb 7 2022, 1:59 PM
clang/test/OpenMP/atomic_compare_codegen.cpp
1991

I think the store here is redundant. Is it because I'm using CGF.EmitScalarExpr?

1991

Oh, shoot. load here, instead of store.

tianshilei1992 added inline comments.Feb 7 2022, 2:04 PM
clang/lib/CodeGen/CGStmtOpenMP.cpp
6047

Using D->IgnoreImpCasts() can make sure to avoid the case that char is casted to int in binary operation. However, say, if user writes the following code:

int x;
#pragma omp atomic compare
  x = x > 1.01 ? 1.01 : x;

1.01 here will be casted to 1 by clang, and a warning will be emitted. Because we ignore the implicit cast, in Sema, it is taken as floating point value. However, we already told user that it is casted to 1, which is a little weird to emit an error then.

remove support for floating point values

ABataev added inline comments.Feb 17 2022, 1:16 PM
clang/test/OpenMP/atomic_compare_codegen.cpp
7

The logic behind -fopenmp-simd is to emit the code only for simd specific instructions.

tianshilei1992 marked an inline comment as done.Feb 17 2022, 1:33 PM
tianshilei1992 added inline comments.
clang/test/OpenMP/atomic_compare_codegen.cpp
7

Right. // SIMD-ONLY0-NOT: {{__kmpc|__tgt}} is used here, same as other cases.

tianshilei1992 marked an inline comment as done.Feb 17 2022, 1:34 PM
ABataev accepted this revision.Feb 21 2022, 12:17 PM

LG with a nit

clang/include/clang/AST/StmtOpenMP.h
2970

Fix comment

This revision is now accepted and ready to land.Feb 21 2022, 12:17 PM

LG with a nit

Actually I got two question in the inline comments. Can you please take a look?

tianshilei1992 retitled this revision from [Clang]OpenMP] Add the codegen support for `atomic compare` to [Clang][OpenMP] Add the codegen support for `atomic compare`.Feb 21 2022, 12:39 PM

LG with a nit

Actually I got two question in the inline comments. Can you please take a look?

Could you point me?

LG with a nit

Actually I got two question in the inline comments. Can you please take a look?

Could you point me?

Sure.

clang/lib/CodeGen/CGStmtOpenMP.cpp
6047

@ABataev Here.

clang/test/OpenMP/atomic_compare_codegen.cpp
1991

And here. @ABataev

ABataev added inline comments.Feb 21 2022, 2:00 PM
clang/lib/CodeGen/CGStmtOpenMP.cpp
6047

Sorry, did not understand it was a question. Could you provide a bit more background, did not quite understand problem?

clang/test/OpenMP/atomic_compare_codegen.cpp
1991

Yes, because of EmitScalarExpr

tianshilei1992 marked an inline comment as done.Feb 21 2022, 2:14 PM
tianshilei1992 added inline comments.
clang/test/OpenMP/atomic_compare_codegen.cpp
1991

Can we somehow avoid the load here?

ABataev added inline comments.Feb 21 2022, 3:20 PM
clang/test/OpenMP/atomic_compare_codegen.cpp
1991

Do you need EmitScalarExpr?

tianshilei1992 marked an inline comment as done.Feb 21 2022, 3:33 PM
tianshilei1992 added inline comments.
clang/test/OpenMP/atomic_compare_codegen.cpp
1991

So it is a rvalue scalar here. What alternative do we have then?

tianshilei1992 marked an inline comment as done.

fix comments

tianshilei1992 marked an inline comment as done.Feb 21 2022, 3:46 PM
tianshilei1992 added inline comments.
clang/lib/CodeGen/CGStmtOpenMP.cpp
6047

I have a better explain in another inline comment in Sema.

clang/lib/Sema/SemaOpenMP.cpp
11231

Here we do D->IgnoreImpCasts(), as shown in the code. The reason is, if we have two chars, say char a, b, and when a > b, clang also inserts an implicit cast from char to int for a and b. We want the actual type here otherwise in the IR builder, the type of D doesn't match X's, causing issues.
However, that IgnoreImpCasts here can cause another issue. Let's say we have the following code:

int x;
#pragma omp atomic compare
  x = x > 1.01 ? 1.01 : x;

clang will also insert an implicit cast from floating point value to integer for the scalar value 1.01 (corresponding to D in the code), and also emits a warning saying the floating point value has been cast to integer or something like that. Because we have the IgnoreImpCasts now, it will fail the type check because D now is of floating point type, and we will emit an error to user.
For users, it might be confusing because on one hand we emit a warning saying the floating point value has been casted, and on the other hand in Sema we tell users it is expected an integer. Users might be thinking, you already cast it to integer, why do you tell me it's a floating point value again?

ABataev added inline comments.Feb 21 2022, 4:21 PM
clang/lib/CodeGen/CGStmtOpenMP.cpp
6047

If you don't need it, do not call it. Why do you need this rvalue?

clang/lib/Sema/SemaOpenMP.cpp
11231

Yiu have the resulting type from the whole expression. You van do explicit cast to this resulting type to avoid problems, if I got it correctly

tianshilei1992 marked 3 inline comments as done.Feb 21 2022, 5:24 PM
tianshilei1992 added inline comments.
clang/test/OpenMP/atomic_compare_codegen.cpp
1991

I have to get it passed to IRBuilder, which accepts a llvm::Value *.

ABataev added inline comments.Feb 21 2022, 5:54 PM
clang/test/OpenMP/atomic_compare_codegen.cpp
1991

Modify irbuilder to not require it in some cases.

tianshilei1992 marked an inline comment as done.Feb 21 2022, 6:00 PM
tianshilei1992 added inline comments.
clang/test/OpenMP/atomic_compare_codegen.cpp
1991

You got me wrong. I need to call a function to convert a rvalue Express * to llvm::Value *. CGF.EmitScalarExpr can do it. Actually now I feel the load here can not be avoided as we want a scalar value instead of a pointer.

tianshilei1992 marked an inline comment as done.

final touch before landing

This revision was landed with ongoing or failed builds.Feb 22 2022, 10:01 AM
This revision was automatically updated to reflect the committed changes.