[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
ABataev accepted this revision. ABataev added a comment. This revision is now accepted and ready to land. LG Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120290/new/ https://reviews.llvm.org/D120290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
tianshilei1992 added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120290/new/ https://reviews.llvm.org/D120290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
tianshilei1992 added inline comments.
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:6168
llvm::Value *EVal = CGF.EmitScalarExpr(E);
+ if (auto CI = dyn_cast(EVal))
+EVal = CGF.Builder.CreateIntCast(
ABataev wrote:
> 1. `auto *CI`
> 2. What if this is not a constant, but just a value with int type? Is this
> possible?
> 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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120290/new/
https://reviews.llvm.org/D120290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
ABataev added inline comments.
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:6168
llvm::Value *EVal = CGF.EmitScalarExpr(E);
+ if (auto CI = dyn_cast(EVal))
+EVal = CGF.Builder.CreateIntCast(
1. `auto *CI`
2. What if this is not a constant, but just a value with int type? Is this
possible?
Comment at: clang/lib/Sema/SemaOpenMP.cpp:11796
X = ThenBO->getLHS();
- D = ThenBO->getRHS();
+ D = ThenBO->getRHS()->IgnoreImpCasts();
Same question as before. It is assignment BO, do you really need to remove
casts here?
Comment at: clang/lib/Sema/SemaOpenMP.cpp:11810
if (checkIfTwoExprsAreSame(ContextRef, X, BO->getLHS())) {
-E = BO->getRHS();
+E = BO->getRHS()->IgnoreImpCasts();
} else if (checkIfTwoExprsAreSame(ContextRef, X, BO->getRHS())) {
Maybe, IgnoreImplicitAsWritten?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120290/new/
https://reviews.llvm.org/D120290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
tianshilei1992 updated this revision to Diff 427182. tianshilei1992 added a comment. fix comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120290/new/ https://reviews.llvm.org/D120290 Files: clang/include/clang/AST/StmtOpenMP.h clang/lib/AST/StmtOpenMP.cpp clang/lib/CodeGen/CGStmtOpenMP.cpp clang/lib/Sema/SemaOpenMP.cpp clang/lib/Serialization/ASTReaderStmt.cpp clang/lib/Serialization/ASTWriterStmt.cpp clang/test/OpenMP/atomic_compare_codegen.cpp ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
ABataev added inline comments.
Comment at: clang/lib/Sema/SemaOpenMP.cpp:11663
X = BO->getLHS();
- D = BO->getRHS();
+ D = BO->getRHS()->IgnoreImpCasts();
tianshilei1992 wrote:
> tianshilei1992 wrote:
> > ABataev wrote:
> > > Why do we need to use `IgnoreImpCasts()` here and in other places?
> > 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.
> https://godbolt.org/z/a6WWdx581
> This shows how the AST looks like.
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
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120290/new/
https://reviews.llvm.org/D120290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
tianshilei1992 added a comment.
ping
Comment at: clang/lib/Sema/SemaOpenMP.cpp:11663
X = BO->getLHS();
- D = BO->getRHS();
+ D = BO->getRHS()->IgnoreImpCasts();
tianshilei1992 wrote:
> ABataev wrote:
> > Why do we need to use `IgnoreImpCasts()` here and in other places?
> 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.
https://godbolt.org/z/a6WWdx581
This shows how the AST looks like.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120290/new/
https://reviews.llvm.org/D120290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
tianshilei1992 added inline comments.
Comment at: clang/lib/Sema/SemaOpenMP.cpp:11663
X = BO->getLHS();
- D = BO->getRHS();
+ D = BO->getRHS()->IgnoreImpCasts();
ABataev wrote:
> Why do we need to use `IgnoreImpCasts()` here and in other places?
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120290/new/
https://reviews.llvm.org/D120290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
ABataev added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:11663 X = BO->getLHS(); - D = BO->getRHS(); + D = BO->getRHS()->IgnoreImpCasts(); Why do we need to use `IgnoreImpCasts()` here and in other places? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120290/new/ https://reviews.llvm.org/D120290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
tianshilei1992 updated this revision to Diff 423198. tianshilei1992 marked an inline comment as done. tianshilei1992 added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120290/new/ https://reviews.llvm.org/D120290 Files: clang/include/clang/AST/StmtOpenMP.h clang/lib/AST/StmtOpenMP.cpp clang/lib/CodeGen/CGStmtOpenMP.cpp clang/lib/Sema/SemaOpenMP.cpp clang/lib/Serialization/ASTReaderStmt.cpp clang/lib/Serialization/ASTWriterStmt.cpp clang/test/OpenMP/atomic_compare_codegen.cpp ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
tianshilei1992 marked 3 inline comments as done.
tianshilei1992 added inline comments.
Comment at: clang/include/clang/AST/StmtOpenMP.h:2835-2847
bool IsXLHSInRHSPart = false;
/// Used for 'atomic update' or 'atomic capture' constructs. They may
/// have atomic expressions of forms
/// \code
/// v = x; ;
/// ; v = x;
/// \endcode
tianshilei1992 wrote:
> ABataev wrote:
> > Transform these booleans to bitfields?
> Will do in another patch.
In D123862.
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:6216
- KindsEncountered.contains(OMPC_capture)) {
-IsCompareCapture = true;
Kind = OMPC_compare;
ABataev wrote:
> tianshilei1992 wrote:
> > ABataev wrote:
> > > Can this be fixed in a separate patch?
> > 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`.
> Kind of NFC? Would be good
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120290/new/
https://reviews.llvm.org/D120290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
tianshilei1992 updated this revision to Diff 423132. tianshilei1992 added a comment. Herald added a project: All. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120290/new/ https://reviews.llvm.org/D120290 Files: clang/include/clang/AST/StmtOpenMP.h clang/lib/AST/StmtOpenMP.cpp clang/lib/CodeGen/CGStmtOpenMP.cpp clang/lib/Sema/SemaOpenMP.cpp clang/lib/Serialization/ASTReaderStmt.cpp clang/lib/Serialization/ASTWriterStmt.cpp clang/test/OpenMP/atomic_compare_codegen.cpp ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
ABataev added inline comments.
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:6216
- KindsEncountered.contains(OMPC_capture)) {
-IsCompareCapture = true;
Kind = OMPC_compare;
tianshilei1992 wrote:
> ABataev wrote:
> > Can this be fixed in a separate patch?
> 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`.
Kind of NFC? Would be good
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120290/new/
https://reviews.llvm.org/D120290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
tianshilei1992 added inline comments.
Comment at: clang/include/clang/AST/StmtOpenMP.h:2917-2920
Create(const ASTContext , SourceLocation StartLoc, SourceLocation EndLoc,
ArrayRef Clauses, Stmt *AssociatedStmt, Expr *X, Expr *V,
- Expr *E, Expr *UE, Expr *D, Expr *Cond, bool IsXLHSInRHSPart,
- bool IsPostfixUpdate);
+ Expr *R, Expr *E, Expr *UE, Expr *D, Expr *Cond, bool IsXLHSInRHSPart,
+ bool IsPostfixUpdate, bool IsFailOnly);
ABataev wrote:
> There are too many params already, better to gather them into a struct/class
Sure, we could do that.
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:6216
- KindsEncountered.contains(OMPC_capture)) {
-IsCompareCapture = true;
Kind = OMPC_compare;
ABataev wrote:
> Can this be fixed in a separate patch?
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`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120290/new/
https://reviews.llvm.org/D120290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
ABataev added inline comments.
Comment at: clang/include/clang/AST/StmtOpenMP.h:2917-2920
Create(const ASTContext , SourceLocation StartLoc, SourceLocation EndLoc,
ArrayRef Clauses, Stmt *AssociatedStmt, Expr *X, Expr *V,
- Expr *E, Expr *UE, Expr *D, Expr *Cond, bool IsXLHSInRHSPart,
- bool IsPostfixUpdate);
+ Expr *R, Expr *E, Expr *UE, Expr *D, Expr *Cond, bool IsXLHSInRHSPart,
+ bool IsPostfixUpdate, bool IsFailOnly);
There are too many params already, better to gather them into a struct/class
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:6216
- KindsEncountered.contains(OMPC_capture)) {
-IsCompareCapture = true;
Kind = OMPC_compare;
Can this be fixed in a separate patch?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120290/new/
https://reviews.llvm.org/D120290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
tianshilei1992 updated this revision to Diff 411688. tianshilei1992 marked an inline comment as done. tianshilei1992 added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120290/new/ https://reviews.llvm.org/D120290 Files: clang/include/clang/AST/StmtOpenMP.h clang/lib/AST/StmtOpenMP.cpp clang/lib/CodeGen/CGStmtOpenMP.cpp clang/lib/Sema/SemaOpenMP.cpp clang/lib/Serialization/ASTReaderStmt.cpp clang/lib/Serialization/ASTWriterStmt.cpp clang/test/OpenMP/atomic_compare_codegen.cpp ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
tianshilei1992 marked an inline comment as done.
tianshilei1992 added inline comments.
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:6067
+ if (VPtr) {
+VOpVal = {VPtr, VPtr->getType()->getPointerElementType(),
+ V->getType().isVolatileQualified(),
nikic wrote:
> 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.
Got it. Thanks for the info.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120290/new/
https://reviews.llvm.org/D120290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
tianshilei1992 updated this revision to Diff 410986. tianshilei1992 marked an inline comment as done. tianshilei1992 added a comment. rebase and fix comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120290/new/ https://reviews.llvm.org/D120290 Files: clang/include/clang/AST/StmtOpenMP.h clang/lib/AST/StmtOpenMP.cpp clang/lib/CodeGen/CGStmtOpenMP.cpp clang/lib/Sema/SemaOpenMP.cpp clang/lib/Serialization/ASTReaderStmt.cpp clang/lib/Serialization/ASTWriterStmt.cpp clang/test/OpenMP/atomic_compare_codegen.cpp ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
nikic added inline comments.
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:6067
+ if (VPtr) {
+VOpVal = {VPtr, VPtr->getType()->getPointerElementType(),
+ V->getType().isVolatileQualified(),
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120290/new/
https://reviews.llvm.org/D120290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
tianshilei1992 marked 2 inline comments as done. tianshilei1992 added inline comments. Comment at: clang/include/clang/AST/StmtOpenMP.h:2835-2847 bool IsXLHSInRHSPart = false; /// Used for 'atomic update' or 'atomic capture' constructs. They may /// have atomic expressions of forms /// \code /// v = x; ; /// ; v = x; /// \endcode ABataev wrote: > Transform these booleans to bitfields? Will do in another patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120290/new/ https://reviews.llvm.org/D120290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
tianshilei1992 marked an inline comment as done. tianshilei1992 added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:11556 X = ThenBO->getLHS(); - D = ThenBO->getRHS(); + D = ThenBO->getRHS()->IgnoreImpCasts(); ABataev wrote: > Need to be careful here, probably may need to cast to resulting type > explicitly to avoid problems between int/float/etc. conversions. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120290/new/ https://reviews.llvm.org/D120290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D120290: [Clang][OpenMP] Add the codegen support for `atomic compare capture`
ABataev added inline comments. Comment at: clang/include/clang/AST/StmtOpenMP.h:2835-2847 bool IsXLHSInRHSPart = false; /// Used for 'atomic update' or 'atomic capture' constructs. They may /// have atomic expressions of forms /// \code /// v = x; ; /// ; v = x; /// \endcode Transform these booleans to bitfields? Comment at: clang/lib/Sema/SemaOpenMP.cpp:11556 X = ThenBO->getLHS(); - D = ThenBO->getRHS(); + D = ThenBO->getRHS()->IgnoreImpCasts(); Need to be careful here, probably may need to cast to resulting type explicitly to avoid problems between int/float/etc. conversions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120290/new/ https://reviews.llvm.org/D120290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
