Page MenuHomePhabricator

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

Authored by tianshilei1992 on Feb 21 2022, 4:18 PM.

Details

Summary

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

Diff Detail

Event Timeline

tianshilei1992 created this revision.Feb 21 2022, 4:18 PM
tianshilei1992 requested review of this revision.Feb 21 2022, 4:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2022, 4:18 PM
tianshilei1992 retitled this revision from [WIP][Clang][OpenMP] Add the codegen support for `atomic compare capture` to [Clang][OpenMP] Add the codegen support for `atomic compare capture`.Feb 21 2022, 10:10 PM
ABataev added inline comments.Feb 22 2022, 5:47 AM
clang/include/clang/AST/StmtOpenMP.h
2829–2855

Transform these booleans to bitfields?

clang/lib/Sema/SemaOpenMP.cpp
11852

Need to be careful here, probably may need to cast to resulting type explicitly to avoid problems between int/float/etc. conversions.

tianshilei1992 marked an inline comment as done.Feb 22 2022, 8:42 AM
tianshilei1992 added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
11852

I think we don't need that as D and E are supposed to be rvalue, and as shown here I only ignore implicit casts for rvalue.

tianshilei1992 marked 2 inline comments as done.Feb 22 2022, 8:42 AM
tianshilei1992 added inline comments.
clang/include/clang/AST/StmtOpenMP.h
2829–2855

Will do in another patch.

nikic added a subscriber: nikic.Feb 23 2022, 7:15 AM
nikic added inline comments.
clang/lib/CodeGen/CGStmtOpenMP.cpp
6215

Please avoid adding new calls to getPointerElementType(). See https://github.com/llvm/llvm-project/commit/b1863d82454b2905db8b492bea0ce8a260362645 for a way to avoid it in this context.

tianshilei1992 marked an inline comment as done.

rebase and fix comments

tianshilei1992 marked an inline comment as done.Feb 23 2022, 6:07 PM
tianshilei1992 added inline comments.
clang/lib/CodeGen/CGStmtOpenMP.cpp
6215

Got it. Thanks for the info.

tianshilei1992 marked an inline comment as done.

rebase

ABataev added inline comments.Feb 28 2022, 6:16 AM
clang/include/clang/AST/StmtOpenMP.h
2935–2942

There are too many params already, better to gather them into a struct/class

clang/lib/CodeGen/CGStmtOpenMP.cpp
6371

Can this be fixed in a separate patch?

tianshilei1992 added inline comments.Feb 28 2022, 9:14 AM
clang/include/clang/AST/StmtOpenMP.h
2935–2942

Sure, we could do that.

clang/lib/CodeGen/CGStmtOpenMP.cpp
6371

Well, I think it's part of this patch because we can't tell if it's compare or compare capture before, but now we can. If we really want to do that, we can have another patch including all changes in this patch related to OMPAtomicDirective.

ABataev added inline comments.Feb 28 2022, 10:11 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
6371

Kind of NFC? Would be good

Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2022, 10:17 AM
tianshilei1992 marked 3 inline comments as done.Apr 15 2022, 10:19 AM
tianshilei1992 added inline comments.
clang/include/clang/AST/StmtOpenMP.h
2829–2855
clang/lib/CodeGen/CGStmtOpenMP.cpp
6371

I think we don't need an extra NFC patch for this part because it's not gonna be very neat w/o some other code in this patch.

tianshilei1992 marked an inline comment as done.

rebase

ABataev added inline comments.Apr 18 2022, 5:40 AM
clang/lib/Sema/SemaOpenMP.cpp
11719

Why do we need to use IgnoreImpCasts() here and in other places?

clang/lib/Sema/SemaOpenMP.cpp
11719

Clang usually inserts implicit casts. For example, if we have:

char a, b, c;
#pragma omp atomic compare capture
  { r = a; if (a > c) { a = b; } }

Clang inserts an implicit cast from char to int for all statements except r = a. In this case, what should be the right solution? I'm not quite sure actually.

ping

clang/lib/Sema/SemaOpenMP.cpp
11719

https://godbolt.org/z/a6WWdx581
This shows how the AST looks like.

ABataev added inline comments.May 4 2022, 8:48 AM
clang/lib/Sema/SemaOpenMP.cpp
11719

I don't see casts to int in a=b (BO points to this expression,right?). The only cast, that maybe should be removed here, is LValueToRValue

tianshilei1992 marked 2 inline comments as done.May 4 2022, 8:34 PM
ABataev added inline comments.May 5 2022, 5:22 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
6188
  1. auto *CI
  2. What if this is not a constant, but just a value with int type? Is this possible?
clang/lib/Sema/SemaOpenMP.cpp
11852

Same question as before. It is assignment BO, do you really need to remove casts here?

11866

Maybe, IgnoreImplicitAsWritten?

tianshilei1992 added inline comments.May 5 2022, 5:42 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
6188

What if this is not a constant, but just a value with int type? Is this possible?

I'm thinking of a similar case.

void foo() {
  int e, d;
  char x, v;
  if (x == e)
    x = d;
}

In this case, there are two casts:

  1. In expression x == e, cast x to int.
  2. In expression x = d, cast d to char.

We cannot ignore both of them. For the 2nd, no question. For the expression x == e, the problem is in e instead of x. e here is always int, further causing issue in codegen.

I'm thinking for those lvalues, we can only set them where they are used as lvalues in case any type lifting. For those rvalues, if their types are not same as x, but compatible with x, we have to insert cast, including truncate, before feed them into codegen. Does it sound good?

fix the wrong use of AtomicOpValue

tianshilei1992 marked 3 inline comments as done.Mon, May 30, 11:03 AM
This revision is now accepted and ready to land.Wed, Jun 1, 9:10 AM
This revision was landed with ongoing or failed builds.Thu, Jun 2, 6:38 PM
This revision was automatically updated to reflect the committed changes.