[PATCH] D118632: [Clang]OpenMP] Add the codegen support for `atomic compare`

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 added inline comments.



Comment at: clang/test/OpenMP/atomic_compare_codegen.cpp:1990
+// CHECK-NEXT:[[DD:%.*]] = alloca double, align 8
+// CHECK-NEXT:[[TMP0:%.*]] = load i8, i8* [[CE]], align 1
+// CHECK-NEXT:[[TMP1:%.*]] = atomicrmw umin i8* [[CX]], i8 [[TMP0]] 
monotonic, align 1

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang]OpenMP] Add the codegen support for `atomic compare`

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 added inline comments.



Comment at: clang/test/OpenMP/atomic_compare_codegen.cpp:1990
+// CHECK-NEXT:[[DD:%.*]] = alloca double, align 8
+// CHECK-NEXT:[[TMP0:%.*]] = load i8, i8* [[CE]], align 1
+// CHECK-NEXT:[[TMP1:%.*]] = atomicrmw umin i8* [[CX]], i8 [[TMP0]] 
monotonic, align 1

tianshilei1992 wrote:
> I think the `store` here is redundant. Is it because I'm using 
> `CGF.EmitScalarExpr`?
Oh, shoot. `load` here, instead of `store`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang]OpenMP] Add the codegen support for `atomic compare`

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 added inline comments.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:6041
+  llvm::Value *EVal = CGF.EmitScalarExpr(E->IgnoreImpCasts());
+  llvm::Value *DVal = D ? CGF.EmitScalarExpr(D->IgnoreImpCasts()) : nullptr;
+

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang]OpenMP] Add the codegen support for `atomic compare`

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 added a comment.

ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang]OpenMP] Add the codegen support for `atomic compare`

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/atomic_compare_codegen.cpp:6
+
+// RUN: %clang_cc1 -verify -triple x86_64-apple-darwin10 -target-cpu core2 
-fopenmp-simd -fopenmp-version=51 -x c -emit-llvm %s -o - | FileCheck 
--check-prefix SIMD-ONLY0 %s
+// %clang_cc1 -fopenmp-simd -fopenmp-version=51 -x c -triple 
x86_64-apple-darwin10 -target-cpu core2 -emit-pch -o %t %s

tianshilei1992 wrote:
> CodeGen for `-fopen-simd` is wrong. It doesn't emit corresponding atomic 
> operations. I'll fix it.
The logic behind -fopenmp-simd is to emit the code only for simd specific 
instructions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang]OpenMP] Add the codegen support for `atomic compare`

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 marked an inline comment as done.
tianshilei1992 added inline comments.



Comment at: clang/test/OpenMP/atomic_compare_codegen.cpp:6
+
+// RUN: %clang_cc1 -verify -triple x86_64-apple-darwin10 -target-cpu core2 
-fopenmp-simd -fopenmp-version=51 -x c -emit-llvm %s -o - | FileCheck 
--check-prefix SIMD-ONLY0 %s
+// %clang_cc1 -fopenmp-simd -fopenmp-version=51 -x c -triple 
x86_64-apple-darwin10 -target-cpu core2 -emit-pch -o %t %s

ABataev wrote:
> tianshilei1992 wrote:
> > CodeGen for `-fopen-simd` is wrong. It doesn't emit corresponding atomic 
> > operations. I'll fix it.
> The logic behind -fopenmp-simd is to emit the code only for simd specific 
> instructions.
Right. `// SIMD-ONLY0-NOT: {{__kmpc|__tgt}}` is used here, same as other cases.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang]OpenMP] Add the codegen support for `atomic compare`

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG with a nit




Comment at: clang/include/clang/AST/StmtOpenMP.h:2970
+  }
+  /// Get
+  Expr *getCondExpr() {

Fix comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang]OpenMP] Add the codegen support for `atomic compare`

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 added a comment.

In D118632#3336094 , @ABataev wrote:

> LG with a nit

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang][OpenMP] Add the codegen support for `atomic compare`

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D118632#3336133 , @tianshilei1992 
wrote:

> In D118632#3336094 , @ABataev wrote:
>
>> LG with a nit
>
> Actually I got two question in the inline comments. Can you please take a 
> look?

Could you point me?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang][OpenMP] Add the codegen support for `atomic compare`

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 added a comment.

In D118632#3336145 , @ABataev wrote:

> In D118632#3336133 , 
> @tianshilei1992 wrote:
>
>> In D118632#3336094 , @ABataev 
>> wrote:
>>
>>> LG with a nit
>>
>> Actually I got two question in the inline comments. Can you please take a 
>> look?
>
> Could you point me?

Sure.




Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:6041
+  llvm::Value *EVal = CGF.EmitScalarExpr(E->IgnoreImpCasts());
+  llvm::Value *DVal = D ? CGF.EmitScalarExpr(D->IgnoreImpCasts()) : nullptr;
+

tianshilei1992 wrote:
> 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.
@ABataev Here.



Comment at: clang/test/OpenMP/atomic_compare_codegen.cpp:1990
+// CHECK-NEXT:[[DD:%.*]] = alloca double, align 8
+// CHECK-NEXT:[[TMP0:%.*]] = load i8, i8* [[CE]], align 1
+// CHECK-NEXT:[[TMP1:%.*]] = atomicrmw umin i8* [[CX]], i8 [[TMP0]] 
monotonic, align 1

tianshilei1992 wrote:
> tianshilei1992 wrote:
> > I think the `store` here is redundant. Is it because I'm using 
> > `CGF.EmitScalarExpr`?
> Oh, shoot. `load` here, instead of `store`.
And here. @ABataev 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang][OpenMP] Add the codegen support for `atomic compare`

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:6041
+  llvm::Value *EVal = CGF.EmitScalarExpr(E->IgnoreImpCasts());
+  llvm::Value *DVal = D ? CGF.EmitScalarExpr(D->IgnoreImpCasts()) : nullptr;
+

tianshilei1992 wrote:
> tianshilei1992 wrote:
> > 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.
> @ABataev Here.
Sorry, did not understand it was a question. Could you provide a bit more 
background, did not quite understand problem?



Comment at: clang/test/OpenMP/atomic_compare_codegen.cpp:1990
+// CHECK-NEXT:[[DD:%.*]] = alloca double, align 8
+// CHECK-NEXT:[[TMP0:%.*]] = load i8, i8* [[CE]], align 1
+// CHECK-NEXT:[[TMP1:%.*]] = atomicrmw umin i8* [[CX]], i8 [[TMP0]] 
monotonic, align 1

tianshilei1992 wrote:
> tianshilei1992 wrote:
> > tianshilei1992 wrote:
> > > I think the `store` here is redundant. Is it because I'm using 
> > > `CGF.EmitScalarExpr`?
> > Oh, shoot. `load` here, instead of `store`.
> And here. @ABataev 
Yes, because of EmitScalarExpr


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang][OpenMP] Add the codegen support for `atomic compare`

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 marked an inline comment as done.
tianshilei1992 added inline comments.



Comment at: clang/test/OpenMP/atomic_compare_codegen.cpp:1990
+// CHECK-NEXT:[[DD:%.*]] = alloca double, align 8
+// CHECK-NEXT:[[TMP0:%.*]] = load i8, i8* [[CE]], align 1
+// CHECK-NEXT:[[TMP1:%.*]] = atomicrmw umin i8* [[CX]], i8 [[TMP0]] 
monotonic, align 1

ABataev wrote:
> tianshilei1992 wrote:
> > tianshilei1992 wrote:
> > > tianshilei1992 wrote:
> > > > I think the `store` here is redundant. Is it because I'm using 
> > > > `CGF.EmitScalarExpr`?
> > > Oh, shoot. `load` here, instead of `store`.
> > And here. @ABataev 
> Yes, because of EmitScalarExpr
Can we somehow avoid the `load` here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang][OpenMP] Add the codegen support for `atomic compare`

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/atomic_compare_codegen.cpp:1990
+// CHECK-NEXT:[[DD:%.*]] = alloca double, align 8
+// CHECK-NEXT:[[TMP0:%.*]] = load i8, i8* [[CE]], align 1
+// CHECK-NEXT:[[TMP1:%.*]] = atomicrmw umin i8* [[CX]], i8 [[TMP0]] 
monotonic, align 1

tianshilei1992 wrote:
> ABataev wrote:
> > tianshilei1992 wrote:
> > > tianshilei1992 wrote:
> > > > tianshilei1992 wrote:
> > > > > I think the `store` here is redundant. Is it because I'm using 
> > > > > `CGF.EmitScalarExpr`?
> > > > Oh, shoot. `load` here, instead of `store`.
> > > And here. @ABataev 
> > Yes, because of EmitScalarExpr
> Can we somehow avoid the `load` here?
Do you need EmitScalarExpr?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang][OpenMP] Add the codegen support for `atomic compare`

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 marked an inline comment as done.
tianshilei1992 added inline comments.



Comment at: clang/test/OpenMP/atomic_compare_codegen.cpp:1990
+// CHECK-NEXT:[[DD:%.*]] = alloca double, align 8
+// CHECK-NEXT:[[TMP0:%.*]] = load i8, i8* [[CE]], align 1
+// CHECK-NEXT:[[TMP1:%.*]] = atomicrmw umin i8* [[CX]], i8 [[TMP0]] 
monotonic, align 1

ABataev wrote:
> tianshilei1992 wrote:
> > ABataev wrote:
> > > tianshilei1992 wrote:
> > > > tianshilei1992 wrote:
> > > > > tianshilei1992 wrote:
> > > > > > I think the `store` here is redundant. Is it because I'm using 
> > > > > > `CGF.EmitScalarExpr`?
> > > > > Oh, shoot. `load` here, instead of `store`.
> > > > And here. @ABataev 
> > > Yes, because of EmitScalarExpr
> > Can we somehow avoid the `load` here?
> Do you need EmitScalarExpr?
So it is a rvalue scalar here. What alternative do we have then?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang][OpenMP] Add the codegen support for `atomic compare`

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 marked an inline comment as done.
tianshilei1992 added inline comments.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:6041
+  llvm::Value *EVal = CGF.EmitScalarExpr(E->IgnoreImpCasts());
+  llvm::Value *DVal = D ? CGF.EmitScalarExpr(D->IgnoreImpCasts()) : nullptr;
+

ABataev wrote:
> tianshilei1992 wrote:
> > tianshilei1992 wrote:
> > > 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.
> > @ABataev Here.
> Sorry, did not understand it was a question. Could you provide a bit more 
> background, did not quite understand problem?
I have a better explain in another inline comment in Sema.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:11231
 C = Cond;
-D = CO->getTrueExpr();
+D = CO->getTrueExpr()->IgnoreImpCasts();
 if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS())) {

Here we do `D->IgnoreImpCasts()`, as shown in the code. The reason is, if we 
have two `char`s, 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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang][OpenMP] Add the codegen support for `atomic compare`

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:6041
+  llvm::Value *EVal = CGF.EmitScalarExpr(E->IgnoreImpCasts());
+  llvm::Value *DVal = D ? CGF.EmitScalarExpr(D->IgnoreImpCasts()) : nullptr;
+

tianshilei1992 wrote:
> ABataev wrote:
> > tianshilei1992 wrote:
> > > tianshilei1992 wrote:
> > > > 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.
> > > @ABataev Here.
> > Sorry, did not understand it was a question. Could you provide a bit more 
> > background, did not quite understand problem?
> I have a better explain in another inline comment in Sema.
If you don't need it, do not call it. Why do you need this rvalue?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:11231
 C = Cond;
-D = CO->getTrueExpr();
+D = CO->getTrueExpr()->IgnoreImpCasts();
 if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS())) {

tianshilei1992 wrote:
> Here we do `D->IgnoreImpCasts()`, as shown in the code. The reason is, if we 
> have two `char`s, 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?
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 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang][OpenMP] Add the codegen support for `atomic compare`

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 marked 3 inline comments as done.
tianshilei1992 added inline comments.



Comment at: clang/test/OpenMP/atomic_compare_codegen.cpp:1990
+// CHECK-NEXT:[[DD:%.*]] = alloca double, align 8
+// CHECK-NEXT:[[TMP0:%.*]] = load i8, i8* [[CE]], align 1
+// CHECK-NEXT:[[TMP1:%.*]] = atomicrmw umin i8* [[CX]], i8 [[TMP0]] 
monotonic, align 1

tianshilei1992 wrote:
> ABataev wrote:
> > tianshilei1992 wrote:
> > > ABataev wrote:
> > > > tianshilei1992 wrote:
> > > > > tianshilei1992 wrote:
> > > > > > tianshilei1992 wrote:
> > > > > > > I think the `store` here is redundant. Is it because I'm using 
> > > > > > > `CGF.EmitScalarExpr`?
> > > > > > Oh, shoot. `load` here, instead of `store`.
> > > > > And here. @ABataev 
> > > > Yes, because of EmitScalarExpr
> > > Can we somehow avoid the `load` here?
> > Do you need EmitScalarExpr?
> So it is a rvalue scalar here. What alternative do we have then?
I have to get it passed to IRBuilder, which accepts a `llvm::Value *`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang][OpenMP] Add the codegen support for `atomic compare`

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/atomic_compare_codegen.cpp:1990
+// CHECK-NEXT:[[DD:%.*]] = alloca double, align 8
+// CHECK-NEXT:[[TMP0:%.*]] = load i8, i8* [[CE]], align 1
+// CHECK-NEXT:[[TMP1:%.*]] = atomicrmw umin i8* [[CX]], i8 [[TMP0]] 
monotonic, align 1

tianshilei1992 wrote:
> tianshilei1992 wrote:
> > ABataev wrote:
> > > tianshilei1992 wrote:
> > > > ABataev wrote:
> > > > > tianshilei1992 wrote:
> > > > > > tianshilei1992 wrote:
> > > > > > > tianshilei1992 wrote:
> > > > > > > > I think the `store` here is redundant. Is it because I'm using 
> > > > > > > > `CGF.EmitScalarExpr`?
> > > > > > > Oh, shoot. `load` here, instead of `store`.
> > > > > > And here. @ABataev 
> > > > > Yes, because of EmitScalarExpr
> > > > Can we somehow avoid the `load` here?
> > > Do you need EmitScalarExpr?
> > So it is a rvalue scalar here. What alternative do we have then?
> I have to get it passed to IRBuilder, which accepts a `llvm::Value *`.
Modify irbuilder to not require it in some cases.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118632: [Clang][OpenMP] Add the codegen support for `atomic compare`

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 marked an inline comment as done.
tianshilei1992 added inline comments.



Comment at: clang/test/OpenMP/atomic_compare_codegen.cpp:1990
+// CHECK-NEXT:[[DD:%.*]] = alloca double, align 8
+// CHECK-NEXT:[[TMP0:%.*]] = load i8, i8* [[CE]], align 1
+// CHECK-NEXT:[[TMP1:%.*]] = atomicrmw umin i8* [[CX]], i8 [[TMP0]] 
monotonic, align 1

ABataev wrote:
> tianshilei1992 wrote:
> > tianshilei1992 wrote:
> > > ABataev wrote:
> > > > tianshilei1992 wrote:
> > > > > ABataev wrote:
> > > > > > tianshilei1992 wrote:
> > > > > > > tianshilei1992 wrote:
> > > > > > > > tianshilei1992 wrote:
> > > > > > > > > I think the `store` here is redundant. Is it because I'm 
> > > > > > > > > using `CGF.EmitScalarExpr`?
> > > > > > > > Oh, shoot. `load` here, instead of `store`.
> > > > > > > And here. @ABataev 
> > > > > > Yes, because of EmitScalarExpr
> > > > > Can we somehow avoid the `load` here?
> > > > Do you need EmitScalarExpr?
> > > So it is a rvalue scalar here. What alternative do we have then?
> > I have to get it passed to IRBuilder, which accepts a `llvm::Value *`.
> Modify irbuilder to not require it in some cases.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118632/new/

https://reviews.llvm.org/D118632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits