[PATCH] D65403: [COFF, ARM64] Reorder handling of aarch64 MSVC builtins

[David Major via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL367323: [COFF][ARM64] Reorder handling of aarch64 MSVC 
builtins (authored by dmajor, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65403?vs=212181&id=212355#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65403

Files:
  cfe/trunk/lib/CodeGen/CGBuiltin.cpp
  cfe/trunk/test/CodeGen/arm64-microsoft-intrinsics.c

Index: cfe/trunk/test/CodeGen/arm64-microsoft-intrinsics.c
===
--- cfe/trunk/test/CodeGen/arm64-microsoft-intrinsics.c
+++ cfe/trunk/test/CodeGen/arm64-microsoft-intrinsics.c
@@ -8,6 +8,10 @@
   return _InterlockedAdd(Addend, Value);
 }
 
+long test_InterlockedAdd_constant(long volatile *Addend) {
+  return _InterlockedAdd(Addend, -1);
+}
+
 // CHECK-LABEL: define {{.*}} i32 @test_InterlockedAdd(i32* %Addend, i32 %Value) {{.*}} {
 // CHECK-MSVC: %[[OLDVAL:[0-9]+]] = atomicrmw add i32* %1, i32 %2 seq_cst
 // CHECK-MSVC: %[[NEWVAL:[0-9]+]] = add i32 %[[OLDVAL:[0-9]+]], %2
Index: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
===
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp
@@ -8014,6 +8014,151 @@
 return Builder.CreateExtractElement(Ops[0], EmitScalarExpr(E->getArg(1)),
 "vgetq_lane");
   }
+  case AArch64::BI_BitScanForward:
+  case AArch64::BI_BitScanForward64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_BitScanForward, E);
+  case AArch64::BI_BitScanReverse:
+  case AArch64::BI_BitScanReverse64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_BitScanReverse, E);
+  case AArch64::BI_InterlockedAnd64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedAnd, E);
+  case AArch64::BI_InterlockedExchange64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchange, E);
+  case AArch64::BI_InterlockedExchangeAdd64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchangeAdd, E);
+  case AArch64::BI_InterlockedExchangeSub64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchangeSub, E);
+  case AArch64::BI_InterlockedOr64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedOr, E);
+  case AArch64::BI_InterlockedXor64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedXor, E);
+  case AArch64::BI_InterlockedDecrement64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedDecrement, E);
+  case AArch64::BI_InterlockedIncrement64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedIncrement, E);
+  case AArch64::BI_InterlockedExchangeAdd8_acq:
+  case AArch64::BI_InterlockedExchangeAdd16_acq:
+  case AArch64::BI_InterlockedExchangeAdd_acq:
+  case AArch64::BI_InterlockedExchangeAdd64_acq:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchangeAdd_acq, E);
+  case AArch64::BI_InterlockedExchangeAdd8_rel:
+  case AArch64::BI_InterlockedExchangeAdd16_rel:
+  case AArch64::BI_InterlockedExchangeAdd_rel:
+  case AArch64::BI_InterlockedExchangeAdd64_rel:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchangeAdd_rel, E);
+  case AArch64::BI_InterlockedExchangeAdd8_nf:
+  case AArch64::BI_InterlockedExchangeAdd16_nf:
+  case AArch64::BI_InterlockedExchangeAdd_nf:
+  case AArch64::BI_InterlockedExchangeAdd64_nf:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchangeAdd_nf, E);
+  case AArch64::BI_InterlockedExchange8_acq:
+  case AArch64::BI_InterlockedExchange16_acq:
+  case AArch64::BI_InterlockedExchange_acq:
+  case AArch64::BI_InterlockedExchange64_acq:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchange_acq, E);
+  case AArch64::BI_InterlockedExchange8_rel:
+  case AArch64::BI_InterlockedExchange16_rel:
+  case AArch64::BI_InterlockedExchange_rel:
+  case AArch64::BI_InterlockedExchange64_rel:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchange_rel, E);
+  case AArch64::BI_InterlockedExchange8_nf:
+  case AArch64::BI_InterlockedExchange16_nf:
+  case AArch64::BI_InterlockedExchange_nf:
+  case AArch64::BI_InterlockedExchange64_nf:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchange_nf, E);
+  case AArch64::BI_InterlockedCompareExchange8_acq:
+  case AArch64::BI_InterlockedCompareExchange16_acq:
+  case AArch64::BI_InterlockedCompareExchange_acq:
+  case AArch64::BI_InterlockedCompareExchange64_acq:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedCompareExchange_acq, E);
+  case AArch64::BI_InterlockedCompareExchange8_rel:
+  case AArch64::BI_InterlockedCompareExchange16_rel:
+  case AArch64::BI_InterlockedCompareExchange_rel:
+  case AArch64::BI_InterlockedCompareExchange64_rel:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedCompareExchange_rel, E);
+  case AArch64::BI_InterlockedCompareExchange8_nf:
+  case AArch64::BI_InterlockedComp

[PATCH] D65403: [COFF, ARM64] Reorder handling of aarch64 MSVC builtins

[Eli Friedman via Phabricator via cfe-commits]

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

LGTM




Comment at: lib/CodeGen/CGBuiltin.cpp:8182
   switch (BuiltinID) {
   default: return nullptr;
   case NEON::BI__builtin_neon_vbsl_v:

dmajor wrote:
> efriedma wrote:
> > I'm a little concerned about the overall code structure here; even if 
> > moving the code for the MSVC builtins is enough to fix those builtins 
> > specifically, is it actually impossible to hit this "default"?  If it is, 
> > can we convert it to an "unreachable"?
> I'm not sure if this question was directed to me... this was a drive-by patch 
> from my end so I'm not familiar with what other types of builtins there might 
> be.
> 
> I should probably mention that I'm hoping to get a fix merged to 9.0 in order 
> to unblock Firefox. Unless someone can tell me that the unreachable is 
> definitely safe, I'd worry about adding instability into the release branch. 
> Perhaps the unreachable could be done in a separate commit only on 10.0 trunk 
> where the tolerance for surprises is generally better.
> 
It should be impossible to reach this normally, I think; any target-specific 
builtin which codegen supports should be handled earlier, and target-specific 
builtins only exist on the relevant target.  The issue would just be a weird 
crash if you add a new builtin Builtins.def without code generation support, 
instead of a nicer "unsupported" error.

But I'm okay taking this as-is without trying to refactor the code to handle 
that gracefully, if we need this for 9.0.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65403



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

[PATCH] D65403: [COFF, ARM64] Reorder handling of aarch64 MSVC builtins

[David Major via Phabricator via cfe-commits]

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



Comment at: lib/CodeGen/CGBuiltin.cpp:8182
   switch (BuiltinID) {
   default: return nullptr;
   case NEON::BI__builtin_neon_vbsl_v:

efriedma wrote:
> I'm a little concerned about the overall code structure here; even if moving 
> the code for the MSVC builtins is enough to fix those builtins specifically, 
> is it actually impossible to hit this "default"?  If it is, can we convert it 
> to an "unreachable"?
I'm not sure if this question was directed to me... this was a drive-by patch 
from my end so I'm not familiar with what other types of builtins there might 
be.

I should probably mention that I'm hoping to get a fix merged to 9.0 in order 
to unblock Firefox. Unless someone can tell me that the unreachable is 
definitely safe, I'd worry about adding instability into the release branch. 
Perhaps the unreachable could be done in a separate commit only on 10.0 trunk 
where the tolerance for surprises is generally better.



Repository:
  rC Clang

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

https://reviews.llvm.org/D65403



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

[PATCH] D65403: [COFF, ARM64] Reorder handling of aarch64 MSVC builtins

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: lib/CodeGen/CGBuiltin.cpp:8182
   switch (BuiltinID) {
   default: return nullptr;
   case NEON::BI__builtin_neon_vbsl_v:

I'm a little concerned about the overall code structure here; even if moving 
the code for the MSVC builtins is enough to fix those builtins specifically, is 
it actually impossible to hit this "default"?  If it is, can we convert it to 
an "unreachable"?


Repository:
  rC Clang

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

https://reviews.llvm.org/D65403



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

[PATCH] D65403: [COFF, ARM64] Reorder handling of aarch64 MSVC builtins

[David Major via Phabricator via cfe-commits]

dmajor created this revision.
dmajor added reviewers: mgrang, efriedma, hans.
Herald added subscribers: kristina, jfb, kristof.beyls, javed.absar.
Herald added a project: clang.

In `CodeGenFunction::EmitAArch64BuiltinExpr()`, bulk move of all of the aarch64 
MSVC-builtin cases to an earlier point in the function (the `// Handle 
non-overloaded intrinsics first.` switch block) in order to avoid an 
unreachable in `GetNeonType()`. The NEON type-overloading logic is not 
appropriate for the Windows builtins.

Fixes https://llvm.org/pr42775


Repository:
  rC Clang

https://reviews.llvm.org/D65403

Files:
  lib/CodeGen/CGBuiltin.cpp
  test/CodeGen/arm64-microsoft-intrinsics.c

Index: test/CodeGen/arm64-microsoft-intrinsics.c
===
--- test/CodeGen/arm64-microsoft-intrinsics.c
+++ test/CodeGen/arm64-microsoft-intrinsics.c
@@ -8,6 +8,10 @@
   return _InterlockedAdd(Addend, Value);
 }
 
+long test_InterlockedAdd_constant(long volatile *Addend) {
+  return _InterlockedAdd(Addend, -1);
+}
+
 // CHECK-LABEL: define {{.*}} i32 @test_InterlockedAdd(i32* %Addend, i32 %Value) {{.*}} {
 // CHECK-MSVC: %[[OLDVAL:[0-9]+]] = atomicrmw add i32* %1, i32 %2 seq_cst
 // CHECK-MSVC: %[[NEWVAL:[0-9]+]] = add i32 %[[OLDVAL:[0-9]+]], %2
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -8011,6 +8011,151 @@
 return Builder.CreateExtractElement(Ops[0], EmitScalarExpr(E->getArg(1)),
 "vgetq_lane");
   }
+  case AArch64::BI_BitScanForward:
+  case AArch64::BI_BitScanForward64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_BitScanForward, E);
+  case AArch64::BI_BitScanReverse:
+  case AArch64::BI_BitScanReverse64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_BitScanReverse, E);
+  case AArch64::BI_InterlockedAnd64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedAnd, E);
+  case AArch64::BI_InterlockedExchange64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchange, E);
+  case AArch64::BI_InterlockedExchangeAdd64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchangeAdd, E);
+  case AArch64::BI_InterlockedExchangeSub64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchangeSub, E);
+  case AArch64::BI_InterlockedOr64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedOr, E);
+  case AArch64::BI_InterlockedXor64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedXor, E);
+  case AArch64::BI_InterlockedDecrement64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedDecrement, E);
+  case AArch64::BI_InterlockedIncrement64:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedIncrement, E);
+  case AArch64::BI_InterlockedExchangeAdd8_acq:
+  case AArch64::BI_InterlockedExchangeAdd16_acq:
+  case AArch64::BI_InterlockedExchangeAdd_acq:
+  case AArch64::BI_InterlockedExchangeAdd64_acq:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchangeAdd_acq, E);
+  case AArch64::BI_InterlockedExchangeAdd8_rel:
+  case AArch64::BI_InterlockedExchangeAdd16_rel:
+  case AArch64::BI_InterlockedExchangeAdd_rel:
+  case AArch64::BI_InterlockedExchangeAdd64_rel:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchangeAdd_rel, E);
+  case AArch64::BI_InterlockedExchangeAdd8_nf:
+  case AArch64::BI_InterlockedExchangeAdd16_nf:
+  case AArch64::BI_InterlockedExchangeAdd_nf:
+  case AArch64::BI_InterlockedExchangeAdd64_nf:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchangeAdd_nf, E);
+  case AArch64::BI_InterlockedExchange8_acq:
+  case AArch64::BI_InterlockedExchange16_acq:
+  case AArch64::BI_InterlockedExchange_acq:
+  case AArch64::BI_InterlockedExchange64_acq:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchange_acq, E);
+  case AArch64::BI_InterlockedExchange8_rel:
+  case AArch64::BI_InterlockedExchange16_rel:
+  case AArch64::BI_InterlockedExchange_rel:
+  case AArch64::BI_InterlockedExchange64_rel:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchange_rel, E);
+  case AArch64::BI_InterlockedExchange8_nf:
+  case AArch64::BI_InterlockedExchange16_nf:
+  case AArch64::BI_InterlockedExchange_nf:
+  case AArch64::BI_InterlockedExchange64_nf:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchange_nf, E);
+  case AArch64::BI_InterlockedCompareExchange8_acq:
+  case AArch64::BI_InterlockedCompareExchange16_acq:
+  case AArch64::BI_InterlockedCompareExchange_acq:
+  case AArch64::BI_InterlockedCompareExchange64_acq:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedCompareExchange_acq, E);
+  case AArch64::BI_InterlockedCompareExchange8_rel:
+  case AArch64::BI_InterlockedCompareExchange16_rel:
+  case AArch64::BI_InterlockedCompareExchange_rel:
+  case AArch64::BI_InterlockedCompareExchange64_rel:
+return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedCompareExchange_rel, E);
+  case AArch64::BI_InterlockedCompar