[PATCH] D92061: [MS] Fix double evaluation of MSVC builtin arguments

[Reid Kleckner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3bd067272671: [MS] Fix double evaluation of MSVC builtin 
arguments (authored by rnk).

Changed prior to commit:
  https://reviews.llvm.org/D92061?vs=307468=307674#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92061

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGen/ms-intrinsics.c

Index: clang/test/CodeGen/ms-intrinsics.c
===
--- clang/test/CodeGen/ms-intrinsics.c
+++ clang/test/CodeGen/ms-intrinsics.c
@@ -147,7 +147,7 @@
 #endif
 
 unsigned char test_BitScanForward(unsigned long *Index, unsigned long Mask) {
-  return _BitScanForward(Index, Mask);
+  return _BitScanForward(++Index, Mask);
 }
 // CHECK: define{{.*}}i8 @test_BitScanForward(i32* {{[a-z_ ]*}}%Index, i32 {{[a-z_ ]*}}%Mask){{.*}}{
 // CHECK:   [[ISNOTZERO:%[a-z0-9._]+]] = icmp eq i32 %Mask, 0
@@ -156,12 +156,13 @@
 // CHECK:   [[RESULT:%[a-z0-9._]+]] = phi i8 [ 0, %[[ISZERO_LABEL:[a-z0-9._]+]] ], [ 1, %[[ISNOTZERO_LABEL]] ]
 // CHECK:   ret i8 [[RESULT]]
 // CHECK:   [[ISNOTZERO_LABEL]]:
+// CHECK:   [[IDXGEP:%[a-z0-9._]+]] = getelementptr inbounds i32, i32* %Index, {{i64|i32}} 1
 // CHECK:   [[INDEX:%[0-9]+]] = tail call i32 @llvm.cttz.i32(i32 %Mask, i1 true)
-// CHECK:   store i32 [[INDEX]], i32* %Index, align 4
+// CHECK:   store i32 [[INDEX]], i32* [[IDXGEP]], align 4
 // CHECK:   br label %[[END_LABEL]]
 
 unsigned char test_BitScanReverse(unsigned long *Index, unsigned long Mask) {
-  return _BitScanReverse(Index, Mask);
+  return _BitScanReverse(++Index, Mask);
 }
 // CHECK: define{{.*}}i8 @test_BitScanReverse(i32* {{[a-z_ ]*}}%Index, i32 {{[a-z_ ]*}}%Mask){{.*}}{
 // CHECK:   [[ISNOTZERO:%[0-9]+]] = icmp eq i32 %Mask, 0
@@ -170,9 +171,10 @@
 // CHECK:   [[RESULT:%[a-z0-9._]+]] = phi i8 [ 0, %[[ISZERO_LABEL:[a-z0-9._]+]] ], [ 1, %[[ISNOTZERO_LABEL]] ]
 // CHECK:   ret i8 [[RESULT]]
 // CHECK:   [[ISNOTZERO_LABEL]]:
+// CHECK:   [[IDXGEP:%[a-z0-9._]+]] = getelementptr inbounds i32, i32* %Index, {{i64|i32}} 1
 // CHECK:   [[REVINDEX:%[0-9]+]] = tail call i32 @llvm.ctlz.i32(i32 %Mask, i1 true)
 // CHECK:   [[INDEX:%[0-9]+]] = xor i32 [[REVINDEX]], 31
-// CHECK:   store i32 [[INDEX]], i32* %Index, align 4
+// CHECK:   store i32 [[INDEX]], i32* [[IDXGEP]], align 4
 // CHECK:   br label %[[END_LABEL]]
 
 #if defined(__x86_64__) || defined(__arm__) || defined(__aarch64__)
@@ -459,19 +461,21 @@
 #endif
 
 short test_InterlockedIncrement16(short volatile *Addend) {
-  return _InterlockedIncrement16(Addend);
+  return _InterlockedIncrement16(++Addend);
 }
 // CHECK: define{{.*}}i16 @test_InterlockedIncrement16(i16*{{[a-z_ ]*}}%Addend){{.*}}{
-// CHECK: [[TMP:%[0-9]+]] = atomicrmw add i16* %Addend, i16 1 seq_cst
+// CHECK: %incdec.ptr = getelementptr inbounds i16, i16* %Addend, {{i64|i32}} 1
+// CHECK: [[TMP:%[0-9]+]] = atomicrmw add i16* %incdec.ptr, i16 1 seq_cst
 // CHECK: [[RESULT:%[0-9]+]] = add i16 [[TMP]], 1
 // CHECK: ret i16 [[RESULT]]
 // CHECK: }
 
 long test_InterlockedIncrement(long volatile *Addend) {
-  return _InterlockedIncrement(Addend);
+  return _InterlockedIncrement(++Addend);
 }
 // CHECK: define{{.*}}i32 @test_InterlockedIncrement(i32*{{[a-z_ ]*}}%Addend){{.*}}{
-// CHECK: [[TMP:%[0-9]+]] = atomicrmw add i32* %Addend, i32 1 seq_cst
+// CHECK: %incdec.ptr = getelementptr inbounds i32, i32* %Addend, {{i64|i32}} 1
+// CHECK: [[TMP:%[0-9]+]] = atomicrmw add i32* %incdec.ptr, i32 1 seq_cst
 // CHECK: [[RESULT:%[0-9]+]] = add i32 [[TMP]], 1
 // CHECK: ret i32 [[RESULT]]
 // CHECK: }
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -4120,10 +4120,7 @@
llvm::AtomicOrdering ,
llvm::SyncScope::ID );
 
-private:
   enum class MSVCIntrin;
-
-public:
   llvm::Value *EmitMSVCBuiltinExpr(MSVCIntrin BuiltinID, const CallExpr *E);
 
   llvm::Value *EmitBuiltinAvailable(const VersionTuple );
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -1011,16 +1011,342 @@
   __fastfail,
 };
 
+static Optional
+translateArmToMsvcIntrin(unsigned BuiltinID) {
+  using MSVCIntrin = CodeGenFunction::MSVCIntrin;
+  switch (BuiltinID) {
+  default:
+return None;
+  case ARM::BI_BitScanForward:
+  case ARM::BI_BitScanForward64:
+return MSVCIntrin::_BitScanForward;
+  case ARM::BI_BitScanReverse:
+  case ARM::BI_BitScanReverse64:
+return MSVCIntrin::_BitScanReverse;
+  case ARM::BI_InterlockedAnd64:
+return MSVCIntrin::_InterlockedAnd;
+  case ARM::BI_InterlockedExchange64:
+return 

[PATCH] D92061: [MS] Fix double evaluation of MSVC builtin arguments

[Reid Kleckner via Phabricator via cfe-commits]

rnk marked an inline comment as done.
rnk added a comment.

Thanks!




Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1019
+  default:
+break;
+  case ARM::BI_BitScanForward:

thakis wrote:
> Maybe `return None` here and LLVM_UNREACHABLE at the bottom?
Sure, why not. Why do you like this version better?



Comment at: clang/lib/CodeGen/CodeGenFunction.h:4126
+  llvm::Value *EmitEvaluatedMSVCBuiltin(MSVCIntrin BuiltinID, const CallExpr 
*E,
+ArrayRef Ops);
 

thakis wrote:
> ...where's the definition of this function? I can't see calls either. I guess 
> this is a remnant from an earlier approach?
Yeah, it is. I'll zap it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92061

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

[PATCH] D92061: [MS] Fix double evaluation of MSVC builtin arguments

[Nico Weber via Phabricator via cfe-commits]

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

Nice!




Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1019
+  default:
+break;
+  case ARM::BI_BitScanForward:

Maybe `return None` here and LLVM_UNREACHABLE at the bottom?



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1165
+  default:
+break;
+  case AArch64::BI_BitScanForward:

same suggestion



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1311
+  default:
+break;
+  case clang::X86::BI_BitScanForward:

Same suggestion



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7175
 
+  // Handle MSVC intrinsics before argument evaluation.
+  if (Optional MsvcIntId = translateArmToMsvcIntrin(BuiltinID))

This comment could answer "why" too: "...because EmitSMVCBuiltinExpr() 
evaluates arguments and they shouldn't be evaluated twice" (maybe worded better)



Comment at: clang/lib/CodeGen/CodeGenFunction.h:4126
+  llvm::Value *EmitEvaluatedMSVCBuiltin(MSVCIntrin BuiltinID, const CallExpr 
*E,
+ArrayRef Ops);
 

...where's the definition of this function? I can't see calls either. I guess 
this is a remnant from an earlier approach?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92061

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

[PATCH] D92061: [MS] Fix double evaluation of MSVC builtin arguments

[Reid Kleckner via Phabricator via cfe-commits]

rnk created this revision.
rnk added reviewers: hans, thakis, rsmith, akhuang.
Herald added a subscriber: jfb.
Herald added a project: clang.
rnk requested review of this revision.

This code got quite twisted because we consider some MSVC builtins to be
target agnostic, and some to be target specific. Target specific
intrinsics have a pattern of doing up-front argument evaluation, while
general intrinsics do not evaluate their arguments up front. As we tried
to share codepaths between the target-specific and target-agnostic
handling, we ended up doing double evaluation.

Instead, have each target handle MSVC intrinsics consistently before up
front argument evaluation. This requires passing less data around and is
more consistent with target independent intrinsic handling.

See D50979  for past examples of this bug. I 
noticed this while looking
into adding some more intrinsics.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92061

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGen/ms-intrinsics.c

Index: clang/test/CodeGen/ms-intrinsics.c
===
--- clang/test/CodeGen/ms-intrinsics.c
+++ clang/test/CodeGen/ms-intrinsics.c
@@ -147,7 +147,7 @@
 #endif
 
 unsigned char test_BitScanForward(unsigned long *Index, unsigned long Mask) {
-  return _BitScanForward(Index, Mask);
+  return _BitScanForward(++Index, Mask);
 }
 // CHECK: define{{.*}}i8 @test_BitScanForward(i32* {{[a-z_ ]*}}%Index, i32 {{[a-z_ ]*}}%Mask){{.*}}{
 // CHECK:   [[ISNOTZERO:%[a-z0-9._]+]] = icmp eq i32 %Mask, 0
@@ -156,12 +156,13 @@
 // CHECK:   [[RESULT:%[a-z0-9._]+]] = phi i8 [ 0, %[[ISZERO_LABEL:[a-z0-9._]+]] ], [ 1, %[[ISNOTZERO_LABEL]] ]
 // CHECK:   ret i8 [[RESULT]]
 // CHECK:   [[ISNOTZERO_LABEL]]:
+// CHECK:   [[IDXGEP:%[a-z0-9._]+]] = getelementptr inbounds i32, i32* %Index, {{i64|i32}} 1
 // CHECK:   [[INDEX:%[0-9]+]] = tail call i32 @llvm.cttz.i32(i32 %Mask, i1 true)
-// CHECK:   store i32 [[INDEX]], i32* %Index, align 4
+// CHECK:   store i32 [[INDEX]], i32* [[IDXGEP]], align 4
 // CHECK:   br label %[[END_LABEL]]
 
 unsigned char test_BitScanReverse(unsigned long *Index, unsigned long Mask) {
-  return _BitScanReverse(Index, Mask);
+  return _BitScanReverse(++Index, Mask);
 }
 // CHECK: define{{.*}}i8 @test_BitScanReverse(i32* {{[a-z_ ]*}}%Index, i32 {{[a-z_ ]*}}%Mask){{.*}}{
 // CHECK:   [[ISNOTZERO:%[0-9]+]] = icmp eq i32 %Mask, 0
@@ -170,9 +171,10 @@
 // CHECK:   [[RESULT:%[a-z0-9._]+]] = phi i8 [ 0, %[[ISZERO_LABEL:[a-z0-9._]+]] ], [ 1, %[[ISNOTZERO_LABEL]] ]
 // CHECK:   ret i8 [[RESULT]]
 // CHECK:   [[ISNOTZERO_LABEL]]:
+// CHECK:   [[IDXGEP:%[a-z0-9._]+]] = getelementptr inbounds i32, i32* %Index, {{i64|i32}} 1
 // CHECK:   [[REVINDEX:%[0-9]+]] = tail call i32 @llvm.ctlz.i32(i32 %Mask, i1 true)
 // CHECK:   [[INDEX:%[0-9]+]] = xor i32 [[REVINDEX]], 31
-// CHECK:   store i32 [[INDEX]], i32* %Index, align 4
+// CHECK:   store i32 [[INDEX]], i32* [[IDXGEP]], align 4
 // CHECK:   br label %[[END_LABEL]]
 
 #if defined(__x86_64__) || defined(__arm__) || defined(__aarch64__)
@@ -459,19 +461,21 @@
 #endif
 
 short test_InterlockedIncrement16(short volatile *Addend) {
-  return _InterlockedIncrement16(Addend);
+  return _InterlockedIncrement16(++Addend);
 }
 // CHECK: define{{.*}}i16 @test_InterlockedIncrement16(i16*{{[a-z_ ]*}}%Addend){{.*}}{
-// CHECK: [[TMP:%[0-9]+]] = atomicrmw add i16* %Addend, i16 1 seq_cst
+// CHECK: %incdec.ptr = getelementptr inbounds i16, i16* %Addend, {{i64|i32}} 1
+// CHECK: [[TMP:%[0-9]+]] = atomicrmw add i16* %incdec.ptr, i16 1 seq_cst
 // CHECK: [[RESULT:%[0-9]+]] = add i16 [[TMP]], 1
 // CHECK: ret i16 [[RESULT]]
 // CHECK: }
 
 long test_InterlockedIncrement(long volatile *Addend) {
-  return _InterlockedIncrement(Addend);
+  return _InterlockedIncrement(++Addend);
 }
 // CHECK: define{{.*}}i32 @test_InterlockedIncrement(i32*{{[a-z_ ]*}}%Addend){{.*}}{
-// CHECK: [[TMP:%[0-9]+]] = atomicrmw add i32* %Addend, i32 1 seq_cst
+// CHECK: %incdec.ptr = getelementptr inbounds i32, i32* %Addend, {{i64|i32}} 1
+// CHECK: [[TMP:%[0-9]+]] = atomicrmw add i32* %incdec.ptr, i32 1 seq_cst
 // CHECK: [[RESULT:%[0-9]+]] = add i32 [[TMP]], 1
 // CHECK: ret i32 [[RESULT]]
 // CHECK: }
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -4120,11 +4120,10 @@
llvm::AtomicOrdering ,
llvm::SyncScope::ID );
 
-private:
   enum class MSVCIntrin;
-
-public:
   llvm::Value *EmitMSVCBuiltinExpr(MSVCIntrin BuiltinID, const CallExpr *E);
+  llvm::Value *EmitEvaluatedMSVCBuiltin(MSVCIntrin BuiltinID, const CallExpr *E,
+ArrayRef Ops);
 
   llvm::Value *EmitBuiltinAvailable(const VersionTuple );
 
Index: