[PATCH] D63617: [COFF, ARM64] Fix encoding of __debugbreak

[Tom Tan via Phabricator via cfe-commits]

TomTan abandoned this revision.
TomTan added a comment.

The fix in LLVM was merged as https://reviews.llvm.org/rL364115.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63617



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

[PATCH] D63617: [COFF, ARM64] Fix encoding of __debugbreak

[Tom Tan via Phabricator via cfe-commits]

TomTan added a comment.

In D63617#1552615 , @rnk wrote:

> Even if `BRK #0xF000` is a Windows convention, it's still possible for ISel 
> to select different instructions for different OSs, and I'd prefer to 
> implement it that way.


Ok, did the implementation in LLVM in https://reviews.llvm.org/D63635.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63617



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

[PATCH] D63617: [COFF, ARM64] Fix encoding of __debugbreak

[Tom Tan via Phabricator via cfe-commits]

TomTan added a comment.

In D63617#1552561 , @rnk wrote:

> I think it would be preferable to make `llvm.debugtrap` emit `brk #0xF000` on 
> aarch64-windows-*, so other frontends (Rust etc) get the right behavior by 
> default. Right now, AArch64 doesn't do anything special for DEBUGTRAP, so we 
> get the default behavior of `llvm.trap`.
>
> Is this `BRK #0xF000` convention universal to all OSs, or is it a specific 
> convention for Windows debuggers?


I looked at `llvm.debugtrap` but I am not sure the side-effect of that fix, 
like could there be some other ABI (mingw, etc.) on Windows ARM64 which expects 
DEBUGTRAP to be the same as `Android/Linux`.

I think this `BRK #0xF000` convention is only for Windows debugger and 
exception handler, such as we define `__fastfail` in the same way with 
different operand, not universal for all OSs.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63617



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

[PATCH] D63617: [COFF, ARM64] Fix encoding of __debugbreak

[Tom Tan via Phabricator via cfe-commits]

TomTan created this revision.
TomTan added reviewers: efriedma, rnk.
Herald added subscribers: cfe-commits, kristof.beyls, javed.absar.
Herald added a project: clang.

On Windows ARM64, intrinsic `__debugbreak` should be compiled into `brk 
#0xF000` which is different from llvm intrinsic `debugtrap` as `brk #0`. This 
change fixes this by transforming `__debugbreak` to the expected inline 
assembly in Clang.


Repository:
  rC Clang

https://reviews.llvm.org/D63617

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


Index: test/CodeGen/ms-intrinsics.c
===
--- test/CodeGen/ms-intrinsics.c
+++ test/CodeGen/ms-intrinsics.c
@@ -1379,6 +1379,14 @@
 // CHECK-INTEL: call void asm sideeffect "int $$0x29", "{cx}"(i32 42) 
#[[NORETURN]]
 // CHECK-ARM64: call void asm sideeffect "brk #0xF003", "{w0}"(i32 42) 
#[[NORETURN:[0-9]+]]
 
+void test__debugbreak() {
+  __debugbreak();
+}
+// CHECK_LABEL: define{{.*}} void @test__debugbreak() {
+// CHECK-INTEL: call void @llvm.debugtrap()
+// CHECK-ARM: call void @llvm.debugtrap()
+// CHECK-ARM64: call void asm sideeffect "brk #0xF000", ""()
+
 // Attributes come last.
 
 // CHECK: attributes #[[NORETURN]] = { noreturn{{.*}} }
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -2125,8 +2125,20 @@
   }
   case Builtin::BI__builtin_trap:
 return RValue::get(EmitTrapCall(Intrinsic::trap));
-  case Builtin::BI__debugbreak:
-return RValue::get(EmitTrapCall(Intrinsic::debugtrap));
+  case Builtin::BI__debugbreak: {
+llvm::Triple::ArchType ISA = getTarget().getTriple().getArch();
+switch (ISA) {
+default:
+  return RValue::get(EmitTrapCall(Intrinsic::debugtrap));
+case llvm::Triple::aarch64: {
+  llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy, false);
+  llvm::InlineAsm *IA =
+llvm::InlineAsm::get(FTy, "brk #0xF000", "", /*SideEffects=*/true);
+  llvm::CallInst *CI = Builder.CreateCall(IA);
+  return RValue::get(CI);
+}
+}
+  }
   case Builtin::BI__builtin_unreachable: {
 EmitUnreachable(E->getExprLoc());
 


Index: test/CodeGen/ms-intrinsics.c
===
--- test/CodeGen/ms-intrinsics.c
+++ test/CodeGen/ms-intrinsics.c
@@ -1379,6 +1379,14 @@
 // CHECK-INTEL: call void asm sideeffect "int $$0x29", "{cx}"(i32 42) #[[NORETURN]]
 // CHECK-ARM64: call void asm sideeffect "brk #0xF003", "{w0}"(i32 42) #[[NORETURN:[0-9]+]]
 
+void test__debugbreak() {
+  __debugbreak();
+}
+// CHECK_LABEL: define{{.*}} void @test__debugbreak() {
+// CHECK-INTEL: call void @llvm.debugtrap()
+// CHECK-ARM: call void @llvm.debugtrap()
+// CHECK-ARM64: call void asm sideeffect "brk #0xF000", ""()
+
 // Attributes come last.
 
 // CHECK: attributes #[[NORETURN]] = { noreturn{{.*}} }
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -2125,8 +2125,20 @@
   }
   case Builtin::BI__builtin_trap:
 return RValue::get(EmitTrapCall(Intrinsic::trap));
-  case Builtin::BI__debugbreak:
-return RValue::get(EmitTrapCall(Intrinsic::debugtrap));
+  case Builtin::BI__debugbreak: {
+llvm::Triple::ArchType ISA = getTarget().getTriple().getArch();
+switch (ISA) {
+default:
+  return RValue::get(EmitTrapCall(Intrinsic::debugtrap));
+case llvm::Triple::aarch64: {
+  llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy, false);
+  llvm::InlineAsm *IA =
+llvm::InlineAsm::get(FTy, "brk #0xF000", "", /*SideEffects=*/true);
+  llvm::CallInst *CI = Builder.CreateCall(IA);
+  return RValue::get(CI);
+}
+}
+  }
   case Builtin::BI__builtin_unreachable: {
 EmitUnreachable(E->getExprLoc());
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61225: [COFF, ARM64] Align global symbol by size for ARM64 MSVC ABI

[Tom Tan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL359744: [COFF, ARM64] Align global symbol by size for ARM64 
MSVC ABI (authored by TomTan, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D61225?vs=197208=197680#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D61225

Files:
  cfe/trunk/include/clang/Basic/TargetInfo.h
  cfe/trunk/lib/AST/ASTContext.cpp
  cfe/trunk/lib/Basic/Targets/AArch64.cpp
  cfe/trunk/lib/Basic/Targets/AArch64.h
  cfe/trunk/lib/Basic/Targets/NVPTX.cpp
  cfe/trunk/test/CodeGen/arm64-microsoft-struct-align.cpp

Index: cfe/trunk/include/clang/Basic/TargetInfo.h
===
--- cfe/trunk/include/clang/Basic/TargetInfo.h
+++ cfe/trunk/include/clang/Basic/TargetInfo.h
@@ -542,7 +542,9 @@
 
   /// getMinGlobalAlign - Return the minimum alignment of a global variable,
   /// unless its alignment is explicitly reduced via attributes.
-  unsigned getMinGlobalAlign() const { return MinGlobalAlign; }
+  virtual unsigned getMinGlobalAlign (uint64_t) const {
+return MinGlobalAlign;
+  }
 
   /// Return the largest alignment for which a suitably-sized allocation with
   /// '::operator new(size_t)' is guaranteed to produce a correctly-aligned
Index: cfe/trunk/test/CodeGen/arm64-microsoft-struct-align.cpp
===
--- cfe/trunk/test/CodeGen/arm64-microsoft-struct-align.cpp
+++ cfe/trunk/test/CodeGen/arm64-microsoft-struct-align.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -triple aarch64-windows -ffreestanding -emit-llvm -O0 \
+// RUN: -x c++ -o - %s | FileCheck %s
+
+struct size1 { char str[1]; };
+struct size2 { char str[2]; };
+struct size7 { char str[4]; };
+struct size8 { char str[8]; };
+struct size63 { char str[63]; };
+struct size64 { char str[64]; };
+
+struct size1 s1;
+// CHECK: @"?s1@@3Usize1@@A" = dso_local global %struct.size1 zeroinitializer, align 1
+
+struct size2 s2;
+// CHECK: @"?s2@@3Usize2@@A" = dso_local global %struct.size2 zeroinitializer, align 4
+
+struct size7 s7;
+// CHECK: @"?s7@@3Usize7@@A" = dso_local global %struct.size7 zeroinitializer, align 4
+
+struct size8 s8;
+// CHECK: @"?s8@@3Usize8@@A" = dso_local global %struct.size8 zeroinitializer, align 8
+
+struct size63 s63;
+// CHECK: @"?s63@@3Usize63@@A" = dso_local global %struct.size63 zeroinitializer, align 8
+
+struct size64 s64;
+// CHECK: @"?s64@@3Usize64@@A" = dso_local global %struct.size64 zeroinitializer, align 16
Index: cfe/trunk/lib/AST/ASTContext.cpp
===
--- cfe/trunk/lib/AST/ASTContext.cpp
+++ cfe/trunk/lib/AST/ASTContext.cpp
@@ -1600,8 +1600,10 @@
   if (BaseT.getQualifiers().hasUnaligned())
 Align = Target->getCharWidth();
   if (const auto *VD = dyn_cast(D)) {
-if (VD->hasGlobalStorage() && !ForAlignof)
-  Align = std::max(Align, getTargetInfo().getMinGlobalAlign());
+if (VD->hasGlobalStorage() && !ForAlignof) {
+  uint64_t TypeSize = getTypeSize(T.getTypePtr());
+  Align = std::max(Align, getTargetInfo().getMinGlobalAlign(TypeSize));
+}
   }
 }
 
@@ -2239,7 +2241,8 @@
 /// getAlignOfGlobalVar - Return the alignment in bits that should be given
 /// to a global variable of the specified type.
 unsigned ASTContext::getAlignOfGlobalVar(QualType T) const {
-  return std::max(getTypeAlign(T), getTargetInfo().getMinGlobalAlign());
+  uint64_t TypeSize = getTypeSize(T.getTypePtr());
+  return std::max(getTypeAlign(T), getTargetInfo().getMinGlobalAlign(TypeSize));
 }
 
 /// getAlignOfGlobalVarInChars - Return the alignment in characters that
Index: cfe/trunk/lib/Basic/Targets/AArch64.cpp
===
--- cfe/trunk/lib/Basic/Targets/AArch64.cpp
+++ cfe/trunk/lib/Basic/Targets/AArch64.cpp
@@ -551,6 +551,23 @@
   return CCK_MicrosoftWin64;
 }
 
+unsigned MicrosoftARM64TargetInfo::getMinGlobalAlign(uint64_t TypeSize) const {
+  unsigned Align = WindowsARM64TargetInfo::getMinGlobalAlign(TypeSize);
+
+  // MSVC does size based alignment for arm64 based on alignment section in
+  // below document, replicate that to keep alignment consistent with object
+  // files compiled by MSVC.
+  // https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions
+  if (TypeSize >= 512) {  // TypeSize >= 64 bytes
+Align = std::max(Align, 128u);// align type at least 16 bytes
+  } else if (TypeSize >= 64) {// TypeSize >= 8 bytes
+Align = std::max(Align, 64u); // align type at least 8 butes
+  } else if (TypeSize >= 16) {// TypeSize >= 2 bytes
+Align = std::max(Align, 32u); // align type at least 4 bytes
+  }
+  return Align;
+}
+
 

[PATCH] D61225: [COFF, ARM64] Align global symbol by size for ARM64 MSVC ABI

[Tom Tan via Phabricator via cfe-commits]

TomTan updated this revision to Diff 197208.
TomTan added a comment.
Herald added a subscriber: jholewinski.

Added test cases and also merged this alignment adjustment to 
`getMinGlobalAlign` in `MicrosoftARM64TargetInfo`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61225

Files:
  include/clang/Basic/TargetInfo.h
  lib/AST/ASTContext.cpp
  lib/Basic/Targets/AArch64.cpp
  lib/Basic/Targets/AArch64.h
  lib/Basic/Targets/NVPTX.cpp
  test/CodeGen/arm64-microsoft-struct-align.cpp

Index: test/CodeGen/arm64-microsoft-struct-align.cpp
===
--- test/CodeGen/arm64-microsoft-struct-align.cpp
+++ test/CodeGen/arm64-microsoft-struct-align.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -triple aarch64-windows -ffreestanding -emit-llvm -O0 \
+// RUN: -x c++ -o - %s | FileCheck %s
+
+struct size1 { char str[1]; };
+struct size2 { char str[2]; };
+struct size7 { char str[4]; };
+struct size8 { char str[8]; };
+struct size63 { char str[63]; };
+struct size64 { char str[64]; };
+
+struct size1 s1;
+// CHECK: @"?s1@@3Usize1@@A" = dso_local global %struct.size1 zeroinitializer, align 1
+
+struct size2 s2;
+// CHECK: @"?s2@@3Usize2@@A" = dso_local global %struct.size2 zeroinitializer, align 4
+
+struct size7 s7;
+// CHECK: @"?s7@@3Usize7@@A" = dso_local global %struct.size7 zeroinitializer, align 4
+
+struct size8 s8;
+// CHECK: @"?s8@@3Usize8@@A" = dso_local global %struct.size8 zeroinitializer, align 8
+
+struct size63 s63;
+// CHECK: @"?s63@@3Usize63@@A" = dso_local global %struct.size63 zeroinitializer, align 8
+
+struct size64 s64;
+// CHECK: @"?s64@@3Usize64@@A" = dso_local global %struct.size64 zeroinitializer, align 16
Index: lib/Basic/Targets/NVPTX.cpp
===
--- lib/Basic/Targets/NVPTX.cpp
+++ lib/Basic/Targets/NVPTX.cpp
@@ -119,7 +119,7 @@
   LongAlign = HostTarget->getLongAlign();
   LongLongWidth = HostTarget->getLongLongWidth();
   LongLongAlign = HostTarget->getLongLongAlign();
-  MinGlobalAlign = HostTarget->getMinGlobalAlign();
+  MinGlobalAlign = HostTarget->getMinGlobalAlign(/* TypeSize = */ 0);
   NewAlign = HostTarget->getNewAlign();
   DefaultAlignForAttributeAligned =
   HostTarget->getDefaultAlignForAttributeAligned();
Index: lib/Basic/Targets/AArch64.h
===
--- lib/Basic/Targets/AArch64.h
+++ lib/Basic/Targets/AArch64.h
@@ -128,6 +128,8 @@
 MacroBuilder ) const override;
   TargetInfo::CallingConvKind
   getCallingConvKind(bool ClangABICompat4) const override;
+
+  unsigned getMinGlobalAlign(uint64_t TypeSize) const override;
 };
 
 // ARM64 MinGW target
Index: lib/Basic/Targets/AArch64.cpp
===
--- lib/Basic/Targets/AArch64.cpp
+++ lib/Basic/Targets/AArch64.cpp
@@ -545,6 +545,23 @@
   return CCK_MicrosoftWin64;
 }
 
+unsigned MicrosoftARM64TargetInfo::getMinGlobalAlign(uint64_t TypeSize) const {
+  unsigned Align = WindowsARM64TargetInfo::getMinGlobalAlign(TypeSize);
+
+  // MSVC does size based alignment for arm64 based on alignment section in
+  // below document, replicate that to keep alignment consistent with object
+  // files compiled by MSVC.
+  // https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions
+  if (TypeSize >= 512) {  // TypeSize >= 64 bytes
+Align = std::max(Align, 128u);// align type at least 16 bytes
+  } else if (TypeSize >= 64) {// TypeSize >= 8 bytes
+Align = std::max(Align, 64u); // align type at least 8 butes
+  } else if (TypeSize >= 16) {// TypeSize >= 2 bytes
+Align = std::max(Align, 32u); // align type at least 4 bytes
+  }
+  return Align;
+}
+
 MinGWARM64TargetInfo::MinGWARM64TargetInfo(const llvm::Triple ,
const TargetOptions )
 : WindowsARM64TargetInfo(Triple, Opts) {
Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -1600,8 +1600,10 @@
   if (BaseT.getQualifiers().hasUnaligned())
 Align = Target->getCharWidth();
   if (const auto *VD = dyn_cast(D)) {
-if (VD->hasGlobalStorage() && !ForAlignof)
-  Align = std::max(Align, getTargetInfo().getMinGlobalAlign());
+if (VD->hasGlobalStorage() && !ForAlignof) {
+  uint64_t TypeSize = getTypeSize(T.getTypePtr());
+  Align = std::max(Align, getTargetInfo().getMinGlobalAlign(TypeSize));
+}
   }
 }
 
@@ -2239,7 +2241,8 @@
 /// getAlignOfGlobalVar - Return the alignment in bits that should be given
 /// to a global variable of the specified type.
 unsigned ASTContext::getAlignOfGlobalVar(QualType T) const {
-  return std::max(getTypeAlign(T), getTargetInfo().getMinGlobalAlign());
+  uint64_t 

[PATCH] D61225: [COFF, ARM64] Align global symbol by size for ARM64 MSVC ABI

[Tom Tan via Phabricator via cfe-commits]

TomTan created this revision.
TomTan added a reviewer: efriedma.
Herald added subscribers: cfe-commits, kristof.beyls, javed.absar.
Herald added a project: clang.

According to alignment section in below ARM64 ABI document, MSVC could increase 
alignment of global data based on its total size. Clang doesn't do this. 
Compile the same symbol into different alignments by Clang and MSVC could cause 
link error because some instruction encodings, like 64-bit LDR/STR with 
immediate, require the target to be 8 bytes aligned, and linker could choose 
code stream with such LDR/STR instruction from MSVC and 4 bytes aligned data 
from Clang into final image, which actually cannot be linked together (see 
https://bugs.llvm.org/show_bug.cgi?id=41506 for more details).

https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=vs-2019#alignment


Repository:
  rC Clang

https://reviews.llvm.org/D61225

Files:
  lib/AST/ASTContext.cpp


Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -1600,8 +1600,25 @@
   if (BaseT.getQualifiers().hasUnaligned())
 Align = Target->getCharWidth();
   if (const auto *VD = dyn_cast(D)) {
-if (VD->hasGlobalStorage() && !ForAlignof)
+if (VD->hasGlobalStorage() && !ForAlignof) {
   Align = std::max(Align, getTargetInfo().getMinGlobalAlign());
+
+  // MSVC does size based alignment for arm64 based on alignment 
section
+  // in below document, replicate that to keep alignment consistent 
with
+  // object files compiled by MSVC.
+  // 
https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions
+  if (getTargetInfo().getTriple().getArch() == llvm::Triple::aarch64 &&
+  getTargetInfo().getTriple().getEnvironment() == 
llvm::Triple::MSVC) {
+uint64_t TypeSize = getTypeSize(T.getTypePtr());
+if (TypeSize >= 512) {  // TypeSize >= 64 bytes
+  Align = std::max(Align, 128u);// align type at least 16 bytes
+} else if (TypeSize >= 64) {// TypeSize >= 8 bytes
+  Align = std::max(Align, 64u); // align type at least 8 butes
+} else if (TypeSize >= 16) {// TypeSize >= 2 bytes
+  Align = std::max(Align, 32u); // align type at least 4 bytes
+}
+  }
+}
   }
 }
 


Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -1600,8 +1600,25 @@
   if (BaseT.getQualifiers().hasUnaligned())
 Align = Target->getCharWidth();
   if (const auto *VD = dyn_cast(D)) {
-if (VD->hasGlobalStorage() && !ForAlignof)
+if (VD->hasGlobalStorage() && !ForAlignof) {
   Align = std::max(Align, getTargetInfo().getMinGlobalAlign());
+
+  // MSVC does size based alignment for arm64 based on alignment section
+  // in below document, replicate that to keep alignment consistent with
+  // object files compiled by MSVC.
+  // https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions
+  if (getTargetInfo().getTriple().getArch() == llvm::Triple::aarch64 &&
+  getTargetInfo().getTriple().getEnvironment() == llvm::Triple::MSVC) {
+uint64_t TypeSize = getTypeSize(T.getTypePtr());
+if (TypeSize >= 512) {  // TypeSize >= 64 bytes
+  Align = std::max(Align, 128u);// align type at least 16 bytes
+} else if (TypeSize >= 64) {// TypeSize >= 8 bytes
+  Align = std::max(Align, 64u); // align type at least 8 butes
+} else if (TypeSize >= 16) {// TypeSize >= 2 bytes
+  Align = std::max(Align, 32u); // align type at least 4 bytes
+}
+  }
+}
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

[Tom Tan via Phabricator via cfe-commits]

TomTan added a comment.

In D60349#1479604 , @efriedma wrote:

> > Return info for HFA and HVA is updated
>
> That's helpful, but not really sufficient; according to the AAPCS rules, both 
> "Pod" and "NotPod" from my testcase are HFAs.


Could you provide some more details on why `NotPod` is HFA, according to AAPCS?


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

https://reviews.llvm.org/D60349



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

[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

[Tom Tan via Phabricator via cfe-commits]

TomTan added a comment.

In D60349#140 , @TomTan wrote:

> In D60349#1477183 , @efriedma wrote:
>
> > > For NotPod, it is aggregate which is specific in the document
> >
> > Yes, it's an aggregate which is returned in registers... but it's returned 
> > in integer registers, unlike Pod which is returned in floating-point 
> > registers.
>
>
> `struct Pod` is HFA (Homogenous Floating-Point Aggregates) which is returned 
> in floating-point register, `struct NotPod` is not HFA so still returned in 
> integer registers. The ARM64 ABI document will be updated to reflect this, 
> thanks.


@efriedma Return info for `HFA` and `HVA` is updated in 
https://github.com/MicrosoftDocs/cpp-docs/pull/970.


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

https://reviews.llvm.org/D60349



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

[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

[Tom Tan via Phabricator via cfe-commits]

TomTan added a comment.

In D60349#1477183 , @efriedma wrote:

> > For NotPod, it is aggregate which is specific in the document
>
> Yes, it's an aggregate which is returned in registers... but it's returned in 
> integer registers, unlike Pod which is returned in floating-point registers.


`struct Pod` is HFA (Homogenous Floating-Point Aggregates) which is returned in 
floating-point register, `struct NotPod` is not HFA so still returned in 
integer registers. The ARM64 ABI document will be updated to reflect this, 
thanks.


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

https://reviews.llvm.org/D60349



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

[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

[Tom Tan via Phabricator via cfe-commits]

TomTan added a comment.

In D60349#1476371 , @efriedma wrote:

> It looks like there's some missing documentation in the ARM64 ABI document 
> involving homogeneous aggregates... in particular, it looks like non-POD 
> types are never homogeneous, or something along those lines.  I guess we can 
> address that in a followup, though.
>
> @TomTan could you look into updating the ARM64 ABI documentation?
>
> Testcase:
>
>   struct Pod {
> double b[2];
>   };
>   struct NotAggregate {
> NotAggregate();
> double b[2];
>   };
>   struct NotPod {
> NotAggregate x;
>   };
>   Pod copy(Pod *x) { return *x; }  // ldp d0,d1,[x0]
>   NotAggregate copy(NotAggregate *x) { return *x; } // stp x8,x9,[x0]
>   NotPod copy(NotPod *x) { return *x; } // ldp x0,x1,[x8]
>


@efriedma from your test sample above, which part is missing in the ARM64 ABI 
document? For `NotPod`, it is aggregate which is specific in the document.


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

https://reviews.llvm.org/D60349



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

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

[Tom Tan via Phabricator via cfe-commits]

TomTan added a comment.

In D57915#1393510 , @thakis wrote:

> In D57915#1389722 , @TomTan wrote:
>
> > In D57915#1389560 , @lebedev.ri 
> > wrote:
> >
> > > In D57915#1389549 , @TomTan 
> > > wrote:
> > >
> > > > Added the tests back. Clang IR should not lower these to bswap calls 
> > > > because they are global library functions. It might be slower to make 
> > > > the call to library function than bswap, but this is the same for other 
> > > > architectures supported by Windows. And just redefine global library 
> > > > function triggers link error in some scenarios.
> > >
> > >
> > > I think there is some deeper reasoning is being omitted here.
> > >  //Why// does the fact that those are "global library functions" bans 
> > > clang from lowering them into IR?
> > >  (and thus, "prevents" LLVM form doing optimizations)
> >
> >
> > The current issue could be caused by the implementation of comdat selection 
> > in LLD as below which provides more strict conflict check than MSVC link 
> > does.
>
>
> lld isn't supposed to be more strict than link.exe. lld used to not implement 
> the comdat selection field until recently, so lld got more strict – but it 
> should've gotten only as strict as link.exe, not moreso. Do you have an 
> example where lld is more strict than link.exe?


My previous reply is LLD comdat linking issue was misleading. The current issue 
is that when function declaration (`extern`) and definition (`static inline`) 
shown up in sequence, the function declaration property `extern` wins over 
`static` in the resulting COMDAT, which causes linking issue for both LLD and 
link.exe. But this issue was just exposed by the recent addition of duplicated 
symbols check in LLD.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57915



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

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

[Tom Tan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL353740: [COFF, ARM64] Remove definitions for _byteswap 
library functions (authored by TomTan, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57915?vs=185849=186306#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57915

Files:
  cfe/trunk/lib/Headers/intrin.h
  cfe/trunk/test/Headers/ms-arm64-intrin.cpp


Index: cfe/trunk/test/Headers/ms-arm64-intrin.cpp
===
--- cfe/trunk/test/Headers/ms-arm64-intrin.cpp
+++ cfe/trunk/test/Headers/ms-arm64-intrin.cpp
@@ -14,16 +14,16 @@
 }
 
 unsigned short check_byteswap_ushort(unsigned short val) {
-// CHECK: call i16 @llvm.bswap.i16(i16 %val)
+// CHECK: call i16 @_byteswap_ushort(i16 %val)
   return _byteswap_ushort(val);
 }
 
 unsigned long check_byteswap_ulong(unsigned long val) {
-// CHECK: call i32 @llvm.bswap.i32(i32 %val)
+// CHECK: call i32 @_byteswap_ulong(i32 %val)
   return _byteswap_ulong(val);
 }
 
 unsigned __int64 check_byteswap_uint64(unsigned __int64 val) {
-// CHECK: call i64 @llvm.bswap.i64(i64 %val)
+// CHECK: call i64 @_byteswap_uint64(i64 %val)
   return _byteswap_uint64(val);
 }
Index: cfe/trunk/lib/Headers/intrin.h
===
--- cfe/trunk/lib/Headers/intrin.h
+++ cfe/trunk/lib/Headers/intrin.h
@@ -557,15 +557,9 @@
 __int64 _ReadStatusReg(int);
 void _WriteStatusReg(int, __int64);
 
-static inline unsigned short _byteswap_ushort (unsigned short val) {
-  return __builtin_bswap16(val);
-}
-static inline unsigned long _byteswap_ulong (unsigned long val) {
-  return __builtin_bswap32(val);
-}
-static inline unsigned __int64 _byteswap_uint64 (unsigned __int64 val) {
-  return __builtin_bswap64(val);
-}
+unsigned short __cdecl _byteswap_ushort(unsigned short val);
+unsigned long __cdecl _byteswap_ulong (unsigned long val);
+unsigned __int64 __cdecl _byteswap_uint64(unsigned __int64 val);
 #endif
 
 
/**\


Index: cfe/trunk/test/Headers/ms-arm64-intrin.cpp
===
--- cfe/trunk/test/Headers/ms-arm64-intrin.cpp
+++ cfe/trunk/test/Headers/ms-arm64-intrin.cpp
@@ -14,16 +14,16 @@
 }
 
 unsigned short check_byteswap_ushort(unsigned short val) {
-// CHECK: call i16 @llvm.bswap.i16(i16 %val)
+// CHECK: call i16 @_byteswap_ushort(i16 %val)
   return _byteswap_ushort(val);
 }
 
 unsigned long check_byteswap_ulong(unsigned long val) {
-// CHECK: call i32 @llvm.bswap.i32(i32 %val)
+// CHECK: call i32 @_byteswap_ulong(i32 %val)
   return _byteswap_ulong(val);
 }
 
 unsigned __int64 check_byteswap_uint64(unsigned __int64 val) {
-// CHECK: call i64 @llvm.bswap.i64(i64 %val)
+// CHECK: call i64 @_byteswap_uint64(i64 %val)
   return _byteswap_uint64(val);
 }
Index: cfe/trunk/lib/Headers/intrin.h
===
--- cfe/trunk/lib/Headers/intrin.h
+++ cfe/trunk/lib/Headers/intrin.h
@@ -557,15 +557,9 @@
 __int64 _ReadStatusReg(int);
 void _WriteStatusReg(int, __int64);
 
-static inline unsigned short _byteswap_ushort (unsigned short val) {
-  return __builtin_bswap16(val);
-}
-static inline unsigned long _byteswap_ulong (unsigned long val) {
-  return __builtin_bswap32(val);
-}
-static inline unsigned __int64 _byteswap_uint64 (unsigned __int64 val) {
-  return __builtin_bswap64(val);
-}
+unsigned short __cdecl _byteswap_ushort(unsigned short val);
+unsigned long __cdecl _byteswap_ulong (unsigned long val);
+unsigned __int64 __cdecl _byteswap_uint64(unsigned __int64 val);
 #endif
 
 /**\
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

[Tom Tan via Phabricator via cfe-commits]

TomTan added a comment.

In D57915#1389788 , @efriedma wrote:

> I did some quick testing with MSVC; apparently it inlines the implementations 
> of these functions when optimizations are on.  We definitely want to support 
> inlining these. Since these are commonly used in performance-sensitive code, 
> I'd prefer to implement the required changes to 
> CodeGenFunction::EmitMSVCBuiltinExpr now, rather than chase after weird 
> performance regressions in the future.
>
>  ---
>
> I'm not sure how you could end up with a "duplicate symbols" error from the 
> current implementation, though; these functions are marked "static", so they 
> shouldn't conflict with the real _byteswap_* functions.


It is a great idea to inline these in Clang, could this be tracked for a 
separate bug? The issue blocks Chromium build for Windows ARM64.

Yes, they are marked as `static inline` in `intrin.h` and are expected to link 
fine, but there are cases `stdlib.h` is included before including `intrin.h`, 
the former provides global declaration which seems inherited by the definition 
in `intrin.h`.


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

https://reviews.llvm.org/D57915



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

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

[Tom Tan via Phabricator via cfe-commits]

TomTan added a comment.

In D57915#1389750 , @lebedev.ri wrote:

> In D57915#1389722 , @TomTan wrote:
>
> > In D57915#1389560 , @lebedev.ri 
> > wrote:
> >
> > > In D57915#1389549 , @TomTan 
> > > wrote:
> > >
> > > > Added the tests back. Clang IR should not lower these to bswap calls 
> > > > because they are global library functions. It might be slower to make 
> > > > the call to library function than bswap, but this is the same for other 
> > > > architectures supported by Windows. And just redefine global library 
> > > > function triggers link error in some scenarios.
> > >
> > >
> > > I think there is some deeper reasoning is being omitted here.
> > >  //Why// does the fact that those are "global library functions" bans 
> > > clang from lowering them into IR?
> > >  (and thus, "prevents" LLVM form doing optimizations)
> >
> >
> > The current issue could be caused by the implementation of comdat selection 
> > in LLD as below which provides more strict conflict check than MSVC link 
> > does.
> >
> > These `_bytewap_*` in `intrin.h` were not intended as optimization, 
> > instead, it is expected to be consistent with declarations in `stdlib.h`. 
> > For optimization, it makes sense to enable it for all architectures 
> > supported by Windows, and make sure it works with LLD.
> >
> > https://github.com/llvm-mirror/lld/blob/b6584c3fab115aa46dad27681af7eb3d4e5d2b35/COFF/InputFiles.cpp#L494
>
>
> These functions are also listed in 
> https://github.com/llvm-mirror/clang/blob/c18e7e9007970a3105617f03bc9d1de89fa1a3e1/include/clang/Basic/Builtins.def#L773-L775
>  Are you sure they shouldn't be simply dropped from `lib/Headers/intrin.h`?
>
> I notice they were just added in D56685 , 
> but that review has no commit message, so the problem it addressed is not 
> documented..
>  So as-is this all looks a bit like hand-waving //to me//.


The list in 
https://github.com/llvm-mirror/clang/blob/c18e7e9007970a3105617f03bc9d1de89fa1a3e1/include/clang/Basic/Builtins.def#L773-L775
 doesn't require any implementation on Clang side,it provides below warning if 
they are not declared (include `intrin.h` or `stdlib.h`) before using. So it 
would not be affected.

test.cpp(3,12):  warning: implicitly declaring library function 
'_byteswap_ushort' with type 'unsigned short (unsigned short)' 
[-Wimplicit-function-declaration]

  return _byteswap_ushort(42);
 ^

test.cpp(3,12):  note: include the header  or explicitly provide a 
declaration for '_byteswap_ushort'


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

https://reviews.llvm.org/D57915



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

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

[Tom Tan via Phabricator via cfe-commits]

TomTan added a comment.

In D57915#1389560 , @lebedev.ri wrote:

> In D57915#1389549 , @TomTan wrote:
>
> > Added the tests back. Clang IR should not lower these to bswap calls 
> > because they are global library functions. It might be slower to make the 
> > call to library function than bswap, but this is the same for other 
> > architectures supported by Windows. And just redefine global library 
> > function triggers link error in some scenarios.
>
>
> I think there is some deeper reasoning is being omitted here.
>  //Why// does the fact that those are "global library functions" bans clang 
> from lowering them into IR?
>  (and thus, "prevents" LLVM form doing optimizations)


The current issue could be caused by the implementation of comdat selection in 
LLD as below which provides more strict conflict check than MSVC link does.

These `_bytewap_*` in `intrin.h` were not intended as optimization, instead, it 
is expected to be consistent with declarations in `stdlib.h`. For optimization, 
it makes sense to enable it for all architectures supported by Windows, and 
make sure it works with LLD.

https://github.com/llvm-mirror/lld/blob/b6584c3fab115aa46dad27681af7eb3d4e5d2b35/COFF/InputFiles.cpp#L494


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

https://reviews.llvm.org/D57915



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

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

[Tom Tan via Phabricator via cfe-commits]

TomTan updated this revision to Diff 185849.
TomTan added a comment.

Added the tests back. Clang IR should not lower these to bswap calls because 
they are global library functions. It might be slower to make the call to 
library function than bswap, but this is the same for other architectures 
supported by Windows. And just redefine global library function triggers link 
error in some scenarios.


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

https://reviews.llvm.org/D57915

Files:
  lib/Headers/intrin.h
  test/Headers/ms-arm64-intrin.cpp


Index: test/Headers/ms-arm64-intrin.cpp
===
--- test/Headers/ms-arm64-intrin.cpp
+++ test/Headers/ms-arm64-intrin.cpp
@@ -14,16 +14,16 @@
 }
 
 unsigned short check_byteswap_ushort(unsigned short val) {
-// CHECK: call i16 @llvm.bswap.i16(i16 %val)
+// CHECK: call i16 @_byteswap_ushort(i16 %val)
   return _byteswap_ushort(val);
 }
 
 unsigned long check_byteswap_ulong(unsigned long val) {
-// CHECK: call i32 @llvm.bswap.i32(i32 %val)
+// CHECK: call i32 @_byteswap_ulong(i32 %val)
   return _byteswap_ulong(val);
 }
 
 unsigned __int64 check_byteswap_uint64(unsigned __int64 val) {
-// CHECK: call i64 @llvm.bswap.i64(i64 %val)
+// CHECK: call i64 @_byteswap_uint64(i64 %val)
   return _byteswap_uint64(val);
 }
Index: lib/Headers/intrin.h
===
--- lib/Headers/intrin.h
+++ lib/Headers/intrin.h
@@ -557,15 +557,9 @@
 int _ReadStatusReg(int);
 void _WriteStatusReg(int, int);
 
-static inline unsigned short _byteswap_ushort (unsigned short val) {
-  return __builtin_bswap16(val);
-}
-static inline unsigned long _byteswap_ulong (unsigned long val) {
-  return __builtin_bswap32(val);
-}
-static inline unsigned __int64 _byteswap_uint64 (unsigned __int64 val) {
-  return __builtin_bswap64(val);
-}
+unsigned short __cdecl _byteswap_ushort(unsigned short val);
+unsigned long __cdecl _byteswap_ulong (unsigned long val);
+unsigned __int64 __cdecl _byteswap_uint64(unsigned __int64 val);
 #endif
 
 
/**\


Index: test/Headers/ms-arm64-intrin.cpp
===
--- test/Headers/ms-arm64-intrin.cpp
+++ test/Headers/ms-arm64-intrin.cpp
@@ -14,16 +14,16 @@
 }
 
 unsigned short check_byteswap_ushort(unsigned short val) {
-// CHECK: call i16 @llvm.bswap.i16(i16 %val)
+// CHECK: call i16 @_byteswap_ushort(i16 %val)
   return _byteswap_ushort(val);
 }
 
 unsigned long check_byteswap_ulong(unsigned long val) {
-// CHECK: call i32 @llvm.bswap.i32(i32 %val)
+// CHECK: call i32 @_byteswap_ulong(i32 %val)
   return _byteswap_ulong(val);
 }
 
 unsigned __int64 check_byteswap_uint64(unsigned __int64 val) {
-// CHECK: call i64 @llvm.bswap.i64(i64 %val)
+// CHECK: call i64 @_byteswap_uint64(i64 %val)
   return _byteswap_uint64(val);
 }
Index: lib/Headers/intrin.h
===
--- lib/Headers/intrin.h
+++ lib/Headers/intrin.h
@@ -557,15 +557,9 @@
 int _ReadStatusReg(int);
 void _WriteStatusReg(int, int);
 
-static inline unsigned short _byteswap_ushort (unsigned short val) {
-  return __builtin_bswap16(val);
-}
-static inline unsigned long _byteswap_ulong (unsigned long val) {
-  return __builtin_bswap32(val);
-}
-static inline unsigned __int64 _byteswap_uint64 (unsigned __int64 val) {
-  return __builtin_bswap64(val);
-}
+unsigned short __cdecl _byteswap_ushort(unsigned short val);
+unsigned long __cdecl _byteswap_ulong (unsigned long val);
+unsigned __int64 __cdecl _byteswap_uint64(unsigned __int64 val);
 #endif
 
 /**\
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

[Tom Tan via Phabricator via cfe-commits]

TomTan created this revision.
TomTan added reviewers: mgrang, efriedma, mstorsjo.
Herald added subscribers: cfe-commits, kristof.beyls, javed.absar.
Herald added a project: clang.

_byteswap_* functions are are implemented in below file as normal function
from libucrt.lib and declared in stdlib.h. Define them in intrin.h triggers
lld error "conflicting comdat type" and "duplicate symbols" which was just
added to LLD (https://reviews.llvm.org/D57324).

C:\Program Files (x86)\Windows 
Kits\10\Source\10.0.17763.0\ucrt\stdlib\byteswap.cpp


Repository:
  rC Clang

https://reviews.llvm.org/D57915

Files:
  lib/Headers/intrin.h
  test/Headers/ms-arm64-intrin.cpp


Index: test/Headers/ms-arm64-intrin.cpp
===
--- test/Headers/ms-arm64-intrin.cpp
+++ test/Headers/ms-arm64-intrin.cpp
@@ -12,18 +12,3 @@
 // CHECK: "nop"
   __nop();
 }
-
-unsigned short check_byteswap_ushort(unsigned short val) {
-// CHECK: call i16 @llvm.bswap.i16(i16 %val)
-  return _byteswap_ushort(val);
-}
-
-unsigned long check_byteswap_ulong(unsigned long val) {
-// CHECK: call i32 @llvm.bswap.i32(i32 %val)
-  return _byteswap_ulong(val);
-}
-
-unsigned __int64 check_byteswap_uint64(unsigned __int64 val) {
-// CHECK: call i64 @llvm.bswap.i64(i64 %val)
-  return _byteswap_uint64(val);
-}
Index: lib/Headers/intrin.h
===
--- lib/Headers/intrin.h
+++ lib/Headers/intrin.h
@@ -557,15 +557,9 @@
 int _ReadStatusReg(int);
 void _WriteStatusReg(int, int);
 
-static inline unsigned short _byteswap_ushort (unsigned short val) {
-  return __builtin_bswap16(val);
-}
-static inline unsigned long _byteswap_ulong (unsigned long val) {
-  return __builtin_bswap32(val);
-}
-static inline unsigned __int64 _byteswap_uint64 (unsigned __int64 val) {
-  return __builtin_bswap64(val);
-}
+unsigned short __cdecl _byteswap_ushort(unsigned short val);
+unsigned long __cdecl _byteswap_ulong (unsigned long val);
+unsigned __int64 __cdecl _byteswap_uint64(unsigned __int64 val);
 #endif
 
 
/**\


Index: test/Headers/ms-arm64-intrin.cpp
===
--- test/Headers/ms-arm64-intrin.cpp
+++ test/Headers/ms-arm64-intrin.cpp
@@ -12,18 +12,3 @@
 // CHECK: "nop"
   __nop();
 }
-
-unsigned short check_byteswap_ushort(unsigned short val) {
-// CHECK: call i16 @llvm.bswap.i16(i16 %val)
-  return _byteswap_ushort(val);
-}
-
-unsigned long check_byteswap_ulong(unsigned long val) {
-// CHECK: call i32 @llvm.bswap.i32(i32 %val)
-  return _byteswap_ulong(val);
-}
-
-unsigned __int64 check_byteswap_uint64(unsigned __int64 val) {
-// CHECK: call i64 @llvm.bswap.i64(i64 %val)
-  return _byteswap_uint64(val);
-}
Index: lib/Headers/intrin.h
===
--- lib/Headers/intrin.h
+++ lib/Headers/intrin.h
@@ -557,15 +557,9 @@
 int _ReadStatusReg(int);
 void _WriteStatusReg(int, int);
 
-static inline unsigned short _byteswap_ushort (unsigned short val) {
-  return __builtin_bswap16(val);
-}
-static inline unsigned long _byteswap_ulong (unsigned long val) {
-  return __builtin_bswap32(val);
-}
-static inline unsigned __int64 _byteswap_uint64 (unsigned __int64 val) {
-  return __builtin_bswap64(val);
-}
+unsigned short __cdecl _byteswap_ushort(unsigned short val);
+unsigned long __cdecl _byteswap_ulong (unsigned long val);
+unsigned __int64 __cdecl _byteswap_uint64(unsigned __int64 val);
 #endif
 
 /**\
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57631: [COFF, ARM64] Add ARM64 support for MS intrinsic _fastfail

[Tom Tan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL353337: [COFF, ARM64] Add ARM64 support for MS intrinsic 
_fastfail (authored by TomTan, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57631?vs=184869=185609#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57631

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


Index: cfe/trunk/test/CodeGen/ms-intrinsics.c
===
--- cfe/trunk/test/CodeGen/ms-intrinsics.c
+++ cfe/trunk/test/CodeGen/ms-intrinsics.c
@@ -9,7 +9,7 @@
 // RUN: | FileCheck %s --check-prefixes 
CHECK,CHECK-X64,CHECK-ARM-X64,CHECK-INTEL
 // RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility 
-fms-compatibility-version=17.00 \
 // RUN: -triple aarch64-windows -Oz -emit-llvm %s -o - \
-// RUN: | FileCheck %s --check-prefixes CHECK-ARM-ARM64,CHECK-ARM-X64
+// RUN: | FileCheck %s --check-prefixes 
CHECK-ARM-ARM64,CHECK-ARM-X64,CHECK-ARM64
 
 // intrin.h needs size_t, but -ffreestanding prevents us from getting it from
 // stddef.h.  Work around it with this typedef.
@@ -1342,15 +1342,14 @@
 // CHECK-ARM-ARM64: }
 #endif
 
-#if !defined(__aarch64__)
 void test__fastfail() {
   __fastfail(42);
 }
 // CHECK-LABEL: define{{.*}} void @test__fastfail()
 // CHECK-ARM: call void asm sideeffect "udf #251", "{r0}"(i32 42) 
#[[NORETURN:[0-9]+]]
 // CHECK-INTEL: call void asm sideeffect "int $$0x29", "{cx}"(i32 42) 
#[[NORETURN]]
+// CHECK-ARM64: call void asm sideeffect "brk #0xF003", "{w0}"(i32 42) 
#[[NORETURN:[0-9]+]]
 
 // Attributes come last.
 
 // CHECK: attributes #[[NORETURN]] = { noreturn{{.*}} }
-#endif
Index: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
===
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp
@@ -997,6 +997,9 @@
   Asm = "udf #251";
   Constraints = "{r0}";
   break;
+case llvm::Triple::aarch64:
+  Asm = "brk #0xF003";
+  Constraints = "{w0}";
 }
 llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy, {Int32Ty}, 
false);
 llvm::InlineAsm *IA =


Index: cfe/trunk/test/CodeGen/ms-intrinsics.c
===
--- cfe/trunk/test/CodeGen/ms-intrinsics.c
+++ cfe/trunk/test/CodeGen/ms-intrinsics.c
@@ -9,7 +9,7 @@
 // RUN: | FileCheck %s --check-prefixes CHECK,CHECK-X64,CHECK-ARM-X64,CHECK-INTEL
 // RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility -fms-compatibility-version=17.00 \
 // RUN: -triple aarch64-windows -Oz -emit-llvm %s -o - \
-// RUN: | FileCheck %s --check-prefixes CHECK-ARM-ARM64,CHECK-ARM-X64
+// RUN: | FileCheck %s --check-prefixes CHECK-ARM-ARM64,CHECK-ARM-X64,CHECK-ARM64
 
 // intrin.h needs size_t, but -ffreestanding prevents us from getting it from
 // stddef.h.  Work around it with this typedef.
@@ -1342,15 +1342,14 @@
 // CHECK-ARM-ARM64: }
 #endif
 
-#if !defined(__aarch64__)
 void test__fastfail() {
   __fastfail(42);
 }
 // CHECK-LABEL: define{{.*}} void @test__fastfail()
 // CHECK-ARM: call void asm sideeffect "udf #251", "{r0}"(i32 42) #[[NORETURN:[0-9]+]]
 // CHECK-INTEL: call void asm sideeffect "int $$0x29", "{cx}"(i32 42) #[[NORETURN]]
+// CHECK-ARM64: call void asm sideeffect "brk #0xF003", "{w0}"(i32 42) #[[NORETURN:[0-9]+]]
 
 // Attributes come last.
 
 // CHECK: attributes #[[NORETURN]] = { noreturn{{.*}} }
-#endif
Index: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
===
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp
@@ -997,6 +997,9 @@
   Asm = "udf #251";
   Constraints = "{r0}";
   break;
+case llvm::Triple::aarch64:
+  Asm = "brk #0xF003";
+  Constraints = "{w0}";
 }
 llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy, {Int32Ty}, false);
 llvm::InlineAsm *IA =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57636: [COFF, ARM64] Fix types for _ReadStatusReg, _WriteStatusReg

[Tom Tan via Phabricator via cfe-commits]

TomTan added a comment.

Thanks for finding out and fixing this. Seems there is also issue in expanding 
`_WriteStatusReg` in `CodeGenFunction::EmitAArch64BuiltinExpr`. The last 
argument for `_WriteStatusReg` is __zero extended__ to `__in64`, which is not 
expected (see below link).

https://github.com/llvm-mirror/clang/blob/8070ca12f87e66f76db528c107e9d291f4a91498/lib/CodeGen/CGBuiltin.cpp#L7100


Repository:
  rC Clang

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

https://reviews.llvm.org/D57636



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

[PATCH] D57631: [COFF, ARM64] Add ARM64 support for MS intrinsic _fastfail

[Tom Tan via Phabricator via cfe-commits]

TomTan added a comment.

In D57631#1381806 , @efriedma wrote:

> LGTM


Thanks Eli. Could you please help commit this change when it is ready?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57631



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

[PATCH] D57631: [COFF, ARM64] Add ARM64 support for MS intrinsic _fastfail

[Tom Tan via Phabricator via cfe-commits]

TomTan created this revision.
TomTan added reviewers: rnk, mstorsjo, efriedma.
Herald added subscribers: cfe-commits, kristof.beyls.
Herald added a project: clang.

The MSDN document was also updated to reflect this, but it probably will take a 
few days to show in below link.

https://docs.microsoft.com/en-us/cpp/intrinsics/fastfail


Repository:
  rC Clang

https://reviews.llvm.org/D57631

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


Index: test/CodeGen/ms-intrinsics.c
===
--- test/CodeGen/ms-intrinsics.c
+++ test/CodeGen/ms-intrinsics.c
@@ -9,7 +9,7 @@
 // RUN: | FileCheck %s --check-prefixes 
CHECK,CHECK-X64,CHECK-ARM-X64,CHECK-INTEL
 // RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility 
-fms-compatibility-version=17.00 \
 // RUN: -triple aarch64-windows -Oz -emit-llvm %s -o - \
-// RUN: | FileCheck %s --check-prefixes CHECK-ARM-ARM64,CHECK-ARM-X64
+// RUN: | FileCheck %s --check-prefixes 
CHECK-ARM-ARM64,CHECK-ARM-X64,CHECK-ARM64
 
 // intrin.h needs size_t, but -ffreestanding prevents us from getting it from
 // stddef.h.  Work around it with this typedef.
@@ -1342,15 +1342,14 @@
 // CHECK-ARM-ARM64: }
 #endif
 
-#if !defined(__aarch64__)
 void test__fastfail() {
   __fastfail(42);
 }
 // CHECK-LABEL: define{{.*}} void @test__fastfail()
 // CHECK-ARM: call void asm sideeffect "udf #251", "{r0}"(i32 42) 
#[[NORETURN:[0-9]+]]
 // CHECK-INTEL: call void asm sideeffect "int $$0x29", "{cx}"(i32 42) 
#[[NORETURN]]
+// CHECK-ARM64: call void asm sideeffect "brk #0xF003", "{w0}"(i32 42) 
#[[NORETURN:[0-9]+]]
 
 // Attributes come last.
 
 // CHECK: attributes #[[NORETURN]] = { noreturn{{.*}} }
-#endif
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -995,6 +995,9 @@
   Asm = "udf #251";
   Constraints = "{r0}";
   break;
+case llvm::Triple::aarch64:
+  Asm = "brk #0xF003";
+  Constraints = "{w0}";
 }
 llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy, {Int32Ty}, 
false);
 llvm::InlineAsm *IA =


Index: test/CodeGen/ms-intrinsics.c
===
--- test/CodeGen/ms-intrinsics.c
+++ test/CodeGen/ms-intrinsics.c
@@ -9,7 +9,7 @@
 // RUN: | FileCheck %s --check-prefixes CHECK,CHECK-X64,CHECK-ARM-X64,CHECK-INTEL
 // RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility -fms-compatibility-version=17.00 \
 // RUN: -triple aarch64-windows -Oz -emit-llvm %s -o - \
-// RUN: | FileCheck %s --check-prefixes CHECK-ARM-ARM64,CHECK-ARM-X64
+// RUN: | FileCheck %s --check-prefixes CHECK-ARM-ARM64,CHECK-ARM-X64,CHECK-ARM64
 
 // intrin.h needs size_t, but -ffreestanding prevents us from getting it from
 // stddef.h.  Work around it with this typedef.
@@ -1342,15 +1342,14 @@
 // CHECK-ARM-ARM64: }
 #endif
 
-#if !defined(__aarch64__)
 void test__fastfail() {
   __fastfail(42);
 }
 // CHECK-LABEL: define{{.*}} void @test__fastfail()
 // CHECK-ARM: call void asm sideeffect "udf #251", "{r0}"(i32 42) #[[NORETURN:[0-9]+]]
 // CHECK-INTEL: call void asm sideeffect "int $$0x29", "{cx}"(i32 42) #[[NORETURN]]
+// CHECK-ARM64: call void asm sideeffect "brk #0xF003", "{w0}"(i32 42) #[[NORETURN:[0-9]+]]
 
 // Attributes come last.
 
 // CHECK: attributes #[[NORETURN]] = { noreturn{{.*}} }
-#endif
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -995,6 +995,9 @@
   Asm = "udf #251";
   Constraints = "{r0}";
   break;
+case llvm::Triple::aarch64:
+  Asm = "brk #0xF003";
+  Constraints = "{w0}";
 }
 llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy, {Int32Ty}, false);
 llvm::InlineAsm *IA =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56620: [COFF, ARM64] Declare intrinsics: __nop, _byteswap_[ushort/ulong/uint64]

[Tom Tan via Phabricator via cfe-commits]

TomTan added a comment.

We need full definition for __nop in intrin.h.




Comment at: lib/Headers/intrin.h:569
+unsigned __int64 _byteswap_uint64 (unsigned __int64 val);
+void __nop();
 #endif

efriedma wrote:
> Isn't there already a declaration of __nop in intrin.h?  (Line 100.)
For __nop, we probably need full definition instead of declaration, just like 
__nop for x86/x64 in this file.


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

https://reviews.llvm.org/D56620



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