[PATCH] D46439: Fix incorrect packed aligned structure layout

[Momchil Velikov via Phabricator via cfe-commits]

chill updated this revision to Diff 145687.

https://reviews.llvm.org/D46439

Files:
  lib/Sema/SemaDecl.cpp
  test/Layout/itanium-pack-and-align.cpp


Index: test/Layout/itanium-pack-and-align.cpp
===
--- /dev/null
+++ test/Layout/itanium-pack-and-align.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm-only 
-fdump-record-layouts %s \
+// RUN:| FileCheck %s
+
+struct S {
+  char x;
+  int y;
+} __attribute__((packed, aligned(8)));
+
+struct alignas(8) T {
+  char x;
+  int y;
+} __attribute__((packed));
+
+S s;
+T t;
+// CHECK:  0 | struct T
+// CHECK-NEXT:  0 |   char x
+// CHECK-NEXT:  1 |   int y
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=8,
+// CHECK-NEXT:|  nvsize=8, nvalign=8]
+
+// CHECK:  0 | struct S
+// CHECK-NEXT:  0 |   char x
+// CHECK-NEXT:  1 |   int y
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=8,
+// CHECK-NETX:|  nvsize=8, nvalign=8]
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -15573,6 +15573,10 @@
 if (!Completed)
   Record->completeDefinition();
 
+// Handle attributes before checking the layout.
+if (Attr)
+  ProcessDeclAttributeList(S, Record, Attr);
+
 // We may have deferred checking for a deleted destructor. Check now.
 if (CXXRecordDecl *CXXRecord = dyn_cast(Record)) {
   auto *Dtor = CXXRecord->getDestructor();
@@ -15703,9 +15707,6 @@
   CDecl->setIvarRBraceLoc(RBrac);
 }
   }
-
-  if (Attr)
-ProcessDeclAttributeList(S, Record, Attr);
 }
 
 /// \brief Determine whether the given integral value is representable within


Index: test/Layout/itanium-pack-and-align.cpp
===
--- /dev/null
+++ test/Layout/itanium-pack-and-align.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm-only -fdump-record-layouts %s \
+// RUN:| FileCheck %s
+
+struct S {
+  char x;
+  int y;
+} __attribute__((packed, aligned(8)));
+
+struct alignas(8) T {
+  char x;
+  int y;
+} __attribute__((packed));
+
+S s;
+T t;
+// CHECK:  0 | struct T
+// CHECK-NEXT:  0 |   char x
+// CHECK-NEXT:  1 |   int y
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=8,
+// CHECK-NEXT:|  nvsize=8, nvalign=8]
+
+// CHECK:  0 | struct S
+// CHECK-NEXT:  0 |   char x
+// CHECK-NEXT:  1 |   int y
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=8,
+// CHECK-NETX:|  nvsize=8, nvalign=8]
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -15573,6 +15573,10 @@
 if (!Completed)
   Record->completeDefinition();
 
+// Handle attributes before checking the layout.
+if (Attr)
+  ProcessDeclAttributeList(S, Record, Attr);
+
 // We may have deferred checking for a deleted destructor. Check now.
 if (CXXRecordDecl *CXXRecord = dyn_cast(Record)) {
   auto *Dtor = CXXRecord->getDestructor();
@@ -15703,9 +15707,6 @@
   CDecl->setIvarRBraceLoc(RBrac);
 }
   }
-
-  if (Attr)
-ProcessDeclAttributeList(S, Record, Attr);
 }
 
 /// \brief Determine whether the given integral value is representable within
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46439: Fix incorrect packed aligned structure layout

[Momchil Velikov via Phabricator via cfe-commits]

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

Update: updated comment, added a test.




Comment at: lib/Sema/SemaDecl.cpp:15651
 }
   } else {
 ObjCIvarDecl **ClsFields =

rsmith wrote:
> Do we need to do any attribute processing in this Objective-C interface / 
> implementation / category case?
I think we didn't need it, or else `ProcessDeclAttributeList` would have been 
called with a null `Record` argument.


https://reviews.llvm.org/D46439



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

[PATCH] D46439: Fix incorrect packed aligned structure layout

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Ping?


https://reviews.llvm.org/D46439



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

[PATCH] D46439: [Sema] Fix incorrect packed aligned structure layout

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Thanks a lot!


https://reviews.llvm.org/D46439



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

[PATCH] D46439: [Sema] Fix incorrect packed aligned structure layout

[Momchil Velikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC332843: [Sema] Fix incorrect packed aligned structure layout 
(authored by chill, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D46439?vs=145687&id=147781#toc

Repository:
  rC Clang

https://reviews.llvm.org/D46439

Files:
  lib/Sema/SemaDecl.cpp
  test/Layout/itanium-pack-and-align.cpp


Index: test/Layout/itanium-pack-and-align.cpp
===
--- test/Layout/itanium-pack-and-align.cpp
+++ test/Layout/itanium-pack-and-align.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm-only 
-fdump-record-layouts %s \
+// RUN:| FileCheck %s
+
+struct S {
+  char x;
+  int y;
+} __attribute__((packed, aligned(8)));
+
+struct alignas(8) T {
+  char x;
+  int y;
+} __attribute__((packed));
+
+S s;
+T t;
+// CHECK:  0 | struct T
+// CHECK-NEXT:  0 |   char x
+// CHECK-NEXT:  1 |   int y
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=8,
+// CHECK-NEXT:|  nvsize=8, nvalign=8]
+
+// CHECK:  0 | struct S
+// CHECK-NEXT:  0 |   char x
+// CHECK-NEXT:  1 |   int y
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=8,
+// CHECK-NETX:|  nvsize=8, nvalign=8]
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -15594,6 +15594,10 @@
 if (!Completed)
   Record->completeDefinition();
 
+// Handle attributes before checking the layout.
+if (Attr)
+  ProcessDeclAttributeList(S, Record, Attr);
+
 // We may have deferred checking for a deleted destructor. Check now.
 if (CXXRecordDecl *CXXRecord = dyn_cast(Record)) {
   auto *Dtor = CXXRecord->getDestructor();
@@ -15724,9 +15728,6 @@
   CDecl->setIvarRBraceLoc(RBrac);
 }
   }
-
-  if (Attr)
-ProcessDeclAttributeList(S, Record, Attr);
 }
 
 /// Determine whether the given integral value is representable within


Index: test/Layout/itanium-pack-and-align.cpp
===
--- test/Layout/itanium-pack-and-align.cpp
+++ test/Layout/itanium-pack-and-align.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm-only -fdump-record-layouts %s \
+// RUN:| FileCheck %s
+
+struct S {
+  char x;
+  int y;
+} __attribute__((packed, aligned(8)));
+
+struct alignas(8) T {
+  char x;
+  int y;
+} __attribute__((packed));
+
+S s;
+T t;
+// CHECK:  0 | struct T
+// CHECK-NEXT:  0 |   char x
+// CHECK-NEXT:  1 |   int y
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=8,
+// CHECK-NEXT:|  nvsize=8, nvalign=8]
+
+// CHECK:  0 | struct S
+// CHECK-NEXT:  0 |   char x
+// CHECK-NEXT:  1 |   int y
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=8,
+// CHECK-NETX:|  nvsize=8, nvalign=8]
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -15594,6 +15594,10 @@
 if (!Completed)
   Record->completeDefinition();
 
+// Handle attributes before checking the layout.
+if (Attr)
+  ProcessDeclAttributeList(S, Record, Attr);
+
 // We may have deferred checking for a deleted destructor. Check now.
 if (CXXRecordDecl *CXXRecord = dyn_cast(Record)) {
   auto *Dtor = CXXRecord->getDestructor();
@@ -15724,9 +15728,6 @@
   CDecl->setIvarRBraceLoc(RBrac);
 }
   }
-
-  if (Attr)
-ProcessDeclAttributeList(S, Record, Attr);
 }
 
 /// Determine whether the given integral value is representable within
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38628: Remove unneeded typename from test

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

FYI, there is a similar issue in  
`test/std/utilities/variant/variant.helpers/variant_alternative.fail.cpp` added 
by the 
same commit

git hash: a12318f5ae0aae44eb17f376d3598717b45f7a5f
 "Added failing tests for index out of range for tuple_element> and 
variant_alternative<>"


https://reviews.llvm.org/D38628



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

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Ping?


https://reviews.llvm.org/D46013



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

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill updated this revision to Diff 157662.
chill added a comment.

Fixed to properly examine the canonical type when getting it's unadjusted 
alignment.


https://reviews.llvm.org/D46013

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/RecordLayout.h
  lib/AST/ASTContext.cpp
  lib/AST/RecordLayout.cpp
  lib/AST/RecordLayoutBuilder.cpp
  lib/CodeGen/TargetInfo.cpp
  test/CodeGen/aapcs-align.cc
  test/CodeGen/aapcs64-align.cc
  test/CodeGen/arm-arguments.c

Index: test/CodeGen/arm-arguments.c
===
--- test/CodeGen/arm-arguments.c
+++ test/CodeGen/arm-arguments.c
@@ -211,10 +211,13 @@
 // APCS-GNU: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align {{[0-9]+}} %[[b]], i8* align {{[0-9]+}} %[[c]]
 // APCS-GNU: %[[d:.*]] = bitcast %struct.s35* %[[a]] to <4 x float>*
 // APCS-GNU: load <4 x float>, <4 x float>* %[[d]], align 16
-// AAPCS-LABEL: define arm_aapcscc <4 x float> @f35(i32 %i, %struct.s35* byval align 8, %struct.s35* byval align 8)
-// AAPCS: %[[a:.*]] = alloca %struct.s35, align 16
-// AAPCS: %[[b:.*]] = bitcast %struct.s35* %[[a]] to i8*
-// AAPCS: %[[c:.*]] = bitcast %struct.s35* %0 to i8*
-// AAPCS: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 16 %[[b]], i8* align 8 %[[c]]
-// AAPCS: %[[d:.*]] = bitcast %struct.s35* %[[a]] to <4 x float>*
-// AAPCS: load <4 x float>, <4 x float>* %[[d]], align 16
+
+// AAPCS-LABEL: define arm_aapcscc <4 x float> @f35(i32 %i, %struct.s35* byval align 4 %s1, %struct.s35* byval align 4 %s2)
+// AAPCS: %[[a_addr:.*]] = alloca <4 x float>, align 16
+// AAPCS: %[[b_addr:.*]] = alloca <4 x float>, align 16
+// AAPCS: %[[p1:.*]] = bitcast %struct.s35* %s1 to <4 x float>*
+// AAPCS: %[[a:.*]] = load <4 x float>, <4 x float>* %[[p1]], align 4
+// AAPCS: %[[p2:.*]] = bitcast %struct.s35* %s2 to <4 x float>*
+// AAPCS: %[[b:.*]] = load <4 x float>, <4 x float>* %[[p2]], align 4
+// AAPCS: store <4 x float> %[[a]], <4 x float>* %[[a_addr]], align 16
+// AAPCS: store <4 x float> %[[b]], <4 x float>* %[[b_addr]], align 16
Index: test/CodeGen/aapcs64-align.cc
===
--- /dev/null
+++ test/CodeGen/aapcs64-align.cc
@@ -0,0 +1,103 @@
+// REQUIRES: arm-registered-target
+// RUN: %clang_cc1 -triple aarch64-none-none-eabi \
+// RUN:   -O2 \
+// RUN:   -emit-llvm -o - %s | FileCheck %s
+
+extern "C" {
+
+// Base case, nothing interesting.
+struct S {
+  long x, y;
+};
+
+void f0(long, S);
+void f0m(long, long, long, long, long, S);
+void g0() {
+  S s = {6, 7};
+  f0(1, s);
+  f0m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g0
+// CHECK: call void @f0(i64 1, [2 x i64] [i64 6, i64 7]
+// CHECK: call void @f0m{{.*}}[2 x i64] [i64 6, i64 7]
+// CHECK: declare void @f0(i64, [2 x i64])
+// CHECK: declare void @f0m(i64, i64, i64, i64, i64, [2 x i64])
+
+// Aligned struct, passed according to its natural alignment.
+struct __attribute__((aligned(16))) S16 {
+  long x, y;
+} s16;
+
+void f1(long, S16);
+void f1m(long, long, long, long, long, S16);
+void g1() {
+  S16 s = {6, 7};
+  f1(1, s);
+  f1m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g1
+// CHECK: call void @f1{{.*}}[2 x i64] [i64 6, i64 7]
+// CHECK: call void @f1m{{.*}}[2 x i64] [i64 6, i64 7]
+// CHECK: declare void @f1(i64, [2 x i64])
+// CHECK: declare void @f1m(i64, i64, i64, i64, i64, [2 x i64])
+
+// Increased natural alignment.
+struct SF16 {
+  long x __attribute__((aligned(16)));
+  long y;
+};
+
+void f3(long, SF16);
+void f3m(long, long, long, long, long, SF16);
+void g3() {
+  SF16 s = {6, 7};
+  f3(1, s);
+  f3m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g3
+// CHECK: call void @f3(i64 1, i128 129127208515966861318)
+// CHECK: call void @f3m(i64 1, i64 2, i64 3, i64 4, i64 5, i128 129127208515966861318)
+// CHECK: declare void @f3(i64, i128)
+// CHECK: declare void @f3m(i64, i64, i64, i64, i64, i128)
+
+
+// Packed structure.
+struct  __attribute__((packed)) P {
+  int x;
+  long u;
+};
+
+void f4(int, P);
+void f4m(int, int, int, int, int, P);
+void g4() {
+  P s = {6, 7};
+  f4(1, s);
+  f4m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g4()
+// CHECK: call void @f4(i32 1, [2 x i64] [i64 30064771078, i64 0])
+// CHECK: void @f4m(i32 1, i32 2, i32 3, i32 4, i32 5, [2 x i64] [i64 30064771078, i64 0])
+// CHECK: declare void @f4(i32, [2 x i64])
+// CHECK: declare void @f4m(i32, i32, i32, i32, i32, [2 x i64])
+
+
+// Packed structure, overaligned, same as above.
+struct  __attribute__((packed, aligned(16))) P16 {
+  int x;
+  long y;
+};
+
+void f5(int, P16);
+void f5m(int, int, int, int, int, P16);
+  void g5() {
+P16 s = {6, 7};
+f5(1, s);
+f5m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g5()
+// CHECK: call void @f5(i32 1, [2 x i64] [i64 30064771078, i64 0])
+// CHECK: void @f5m(i32 1, i32 2, i32 3, i32 4, i32 5, [2 x i64] [i64 30064771078, i64 0])
+// CHECK: declare void @f5(i32, [2 x i64])
+// CHECK: declare void @f5m(i32, i32, i32, i32, i32, [2 x i64])
+
+}
Index: t

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

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

Thanks for pointing this!


https://reviews.llvm.org/D46013



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

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC338279: [ARM, AArch64]: Use unadjusted alignment when 
passing composites as arguments (authored by chill, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D46013

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/RecordLayout.h
  lib/AST/ASTContext.cpp
  lib/AST/RecordLayout.cpp
  lib/AST/RecordLayoutBuilder.cpp
  lib/CodeGen/TargetInfo.cpp
  test/CodeGen/aapcs-align.cc
  test/CodeGen/aapcs64-align.cc
  test/CodeGen/arm-arguments.c

Index: include/clang/AST/RecordLayout.h
===
--- include/clang/AST/RecordLayout.h
+++ include/clang/AST/RecordLayout.h
@@ -71,6 +71,10 @@
   // Alignment - Alignment of record in characters.
   CharUnits Alignment;
 
+  // UnadjustedAlignment - Maximum of the alignments of the record members in
+  // characters.
+  CharUnits UnadjustedAlignment;
+
   /// RequiredAlignment - The required alignment of the object.  In the MS-ABI
   /// the __declspec(align()) trumps #pramga pack and must always be obeyed.
   CharUnits RequiredAlignment;
@@ -136,14 +140,16 @@
   CXXRecordLayoutInfo *CXXInfo = nullptr;
 
   ASTRecordLayout(const ASTContext &Ctx, CharUnits size, CharUnits alignment,
+  CharUnits unadjustedAlignment,
   CharUnits requiredAlignment, CharUnits datasize,
   ArrayRef fieldoffsets);
 
   using BaseOffsetsMapTy = CXXRecordLayoutInfo::BaseOffsetsMapTy;
 
   // Constructor for C++ records.
   ASTRecordLayout(const ASTContext &Ctx,
   CharUnits size, CharUnits alignment,
+  CharUnits unadjustedAlignment,
   CharUnits requiredAlignment,
   bool hasOwnVFPtr, bool hasExtendableVFPtr,
   CharUnits vbptroffset,
@@ -170,6 +176,10 @@
   /// getAlignment - Get the record alignment in characters.
   CharUnits getAlignment() const { return Alignment; }
 
+  /// getUnadjustedAlignment - Get the record alignment in characters, before
+  /// alignment adjustement.
+  CharUnits getUnadjustedAlignment() const { return UnadjustedAlignment; }
+
   /// getSize - Get the record size in characters.
   CharUnits getSize() const { return Size; }
 
Index: include/clang/AST/ASTContext.h
===
--- include/clang/AST/ASTContext.h
+++ include/clang/AST/ASTContext.h
@@ -226,6 +226,12 @@
   using TypeInfoMap = llvm::DenseMap;
   mutable TypeInfoMap MemoizedTypeInfo;
 
+  /// A cache from types to unadjusted alignment information. Only ARM and
+  /// AArch64 targets need this information, keeping it separate prevents
+  /// imposing overhead on TypeInfo size.
+  using UnadjustedAlignMap = llvm::DenseMap;
+  mutable UnadjustedAlignMap MemoizedUnadjustedAlign;
+
   /// A cache mapping from CXXRecordDecls to key functions.
   llvm::DenseMap KeyFunctions;
 
@@ -2067,6 +2073,16 @@
   unsigned getTypeAlign(QualType T) const { return getTypeInfo(T).Align; }
   unsigned getTypeAlign(const Type *T) const { return getTypeInfo(T).Align; }
 
+  /// Return the ABI-specified natural alignment of a (complete) type \p T,
+  /// before alignment adjustments, in bits.
+  ///
+  /// This alignment is curently used only by ARM and AArch64 when passing
+  /// arguments of a composite type.
+  unsigned getTypeUnadjustedAlign(QualType T) const {
+return getTypeUnadjustedAlign(T.getTypePtr());
+  }
+  unsigned getTypeUnadjustedAlign(const Type *T) const;
+
   /// Return the ABI-specified alignment of a type, in bits, or 0 if
   /// the type is incomplete and we cannot determine the alignment (for
   /// example, from alignment attributes).
@@ -2077,6 +2093,12 @@
   CharUnits getTypeAlignInChars(QualType T) const;
   CharUnits getTypeAlignInChars(const Type *T) const;
 
+  /// getTypeUnadjustedAlignInChars - Return the ABI-specified alignment of a type,
+  /// in characters, before alignment adjustments. This method does not work on
+  /// incomplete types.
+  CharUnits getTypeUnadjustedAlignInChars(QualType T) const;
+  CharUnits getTypeUnadjustedAlignInChars(const Type *T) const;
+
   // getTypeInfoDataSizeInChars - Return the size of a type, in chars. If the
   // type is a record, its data size is returned.
   std::pair getTypeInfoDataSizeInChars(QualType T) const;
Index: test/CodeGen/arm-arguments.c
===
--- test/CodeGen/arm-arguments.c
+++ test/CodeGen/arm-arguments.c
@@ -211,10 +211,13 @@
 // APCS-GNU: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align {{[0-9]+}} %[[b]], i8* align {{[0-9]+}} %[[c]]
 // APCS-GNU: %[[d:.*]] = bitcast %struct.s35* %[[a]] to <4 x float>*
 // APCS-GNU: load <4 x float>, <4 x float>* %[[d]], align 16
-// AAPCS-LABEL: define arm_aapcscc <4 x float> @f35(i32 %i, %struct.s35* byval align 8, %struct.s35* byval align 8)
-// AAPCS: %[[a:.*]] = alloca %struct.s35, align 16
-/

[PATCH] D42736: [DebugInfo] Improvements to representation of enumeration types (PR36168)

[Momchil Velikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL324490: [DebugInfo] Improvements to representation of 
enumeration types (PR36168) (authored by chill, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D42736?vs=132822&id=133228#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D42736

Files:
  cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
  cfe/trunk/test/CodeGen/debug-info-enum.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-enum-class.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-enum.cpp
  cfe/trunk/test/Modules/ModuleDebugInfo.cpp

Index: cfe/trunk/test/CodeGenCXX/debug-info-enum.cpp
===
--- cfe/trunk/test/CodeGenCXX/debug-info-enum.cpp
+++ cfe/trunk/test/CodeGenCXX/debug-info-enum.cpp
@@ -11,7 +11,7 @@
 // CHECK-SAME:  identifier: "_ZTSN5test11eE"
 // CHECK: [[TEST1]] = !DINamespace(name: "test1"
 // CHECK: [[TEST1_ENUMS]] = !{[[TEST1_E:![0-9]*]]}
-// CHECK: [[TEST1_E]] = !DIEnumerator(name: "E", value: 0)
+// CHECK: [[TEST1_E]] = !DIEnumerator(name: "E", value: 0, isUnsigned: true)
 enum e { E };
 void foo() {
   int v = E;
Index: cfe/trunk/test/CodeGenCXX/debug-info-enum-class.cpp
===
--- cfe/trunk/test/CodeGenCXX/debug-info-enum-class.cpp
+++ cfe/trunk/test/CodeGenCXX/debug-info-enum-class.cpp
@@ -15,20 +15,20 @@
 // CHECK-SAME: baseType: ![[INT:[0-9]+]]
 // CHECK-SAME: size: 32
 // CHECK-NOT:  offset:
-// CHECK-NOT:  flags:
+// CHECK-SAME: flags: DIFlagFixedEnum
 // CHECK-SAME: ){{$}}
 // CHECK: ![[INT]] = !DIBasicType(name: "int"
 // CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "B"
 // CHECK-SAME: line: 4
 // CHECK-SAME: baseType: ![[ULONG:[0-9]+]]
 // CHECK-SAME: size: 64
 // CHECK-NOT:  offset:
-// CHECK-NOT:  flags:
+// CHECK-SAME: flags: DIFlagFixedEnum
 // CHECK-SAME: ){{$}}
 // CHECK: ![[ULONG]] = !DIBasicType(name: "long unsigned int"
 // CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "C"
 // CHECK-SAME: line: 5
-// CHECK-NOT:  baseType:
+// CHECK-SAME: baseType: ![[ULONG:[0-9]+]]
 // CHECK-SAME: size: 32
 // CHECK-NOT:  offset:
 // CHECK-NOT:  flags:
Index: cfe/trunk/test/Modules/ModuleDebugInfo.cpp
===
--- cfe/trunk/test/Modules/ModuleDebugInfo.cpp
+++ cfe/trunk/test/Modules/ModuleDebugInfo.cpp
@@ -48,7 +48,7 @@
 // CHECK: !DICompositeType(tag: DW_TAG_enumeration_type,
 // CHECK-NOT:  name:
 // CHECK-SAME: )
-// CHECK: !DIEnumerator(name: "e5", value: 5)
+// CHECK: !DIEnumerator(name: "e5", value: 5, isUnsigned: true)
 
 // CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
 // no mangled name here yet.
Index: cfe/trunk/test/CodeGen/debug-info-enum.cpp
===
--- cfe/trunk/test/CodeGen/debug-info-enum.cpp
+++ cfe/trunk/test/CodeGen/debug-info-enum.cpp
@@ -0,0 +1,100 @@
+// Test enumeration representation in debuig info metadata:
+// * test value representation for each possible underlying integer type
+// * test the integer type is as expected
+// * test the DW_AT_enum_class attribute is present (resp. absent) as expected.
+
+// RUN: %clang -target x86_64-linux -g -S -emit-llvm -o - %s | FileCheck %s
+
+
+enum class E0 : signed char {
+  A0 = -128,
+  B0 = 127,
+} x0;
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E0"
+// CHECK-SAME: baseType: ![[SCHAR:[0-9]+]]
+// CHECK-SAME: DIFlagFixedEnum
+// CHECK-SAME: elements: ![[ELTS0:[0-9]+]]
+// CHECK: ![[SCHAR]] = !DIBasicType(name: "signed char", size: 8, encoding: DW_ATE_signed_char)
+// CHECK: ![[ELTS0]] = !{![[A0:[0-9]+]], ![[B0:[0-9]+]]}
+// CHECK: ![[A0]] = !DIEnumerator(name: "A0", value: -128)
+// CHECK: ![[B0]] = !DIEnumerator(name: "B0", value: 127)
+
+enum class E1 : unsigned char { A1 = 255 } x1;
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E1"
+// CHECK-SAME: baseType: ![[UCHAR:[0-9]+]]
+// CHECK-SAME: DIFlagFixedEnum
+// CHECK-SAME: elements: ![[ELTS1:[0-9]+]]
+// CHECK: ![[UCHAR]] = !DIBasicType(name: "unsigned char", size: 8, encoding: DW_ATE_unsigned_char)
+// CHECK: ![[ELTS1]] = !{![[A1:[0-9]+]]}
+// CHECK: ![[A1]] = !DIEnumerator(name: "A1", value: 255, isUnsigned: true)
+
+enum class E2 : signed short {
+  A2 = -32768,
+  B2 = 32767,
+} x2;
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E2"
+// CHECK-SAME: baseType: ![[SHORT:[0-9]+]]
+// CHECK-SAME: DIFlagFixedEnum
+// CHECK-SAME: elements: ![[ELTS2:[0-9]+]]
+// CHECK: ![[SHORT]] = !DIBasicType(name: "short", size: 16, encoding: DW_ATE_signed)
+// CHECK: ![[ELTS2]] = !{![[A2:[0-9]+]

[PATCH] D42736: [DebugInfo] Improvements to representation of enumeration types (PR36168)

[Momchil Velikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC324490: [DebugInfo] Improvements to representation of 
enumeration types (PR36168) (authored by chill, committed by ).

Repository:
  rL LLVM

https://reviews.llvm.org/D42736

Files:
  lib/CodeGen/CGDebugInfo.cpp
  test/CodeGen/debug-info-enum.cpp
  test/CodeGenCXX/debug-info-enum-class.cpp
  test/CodeGenCXX/debug-info-enum.cpp
  test/Modules/ModuleDebugInfo.cpp

Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -2492,22 +2492,24 @@
   // Create elements for each enumerator.
   SmallVector Enumerators;
   ED = ED->getDefinition();
+  bool IsSigned = ED->getIntegerType()->isSignedIntegerType();
   for (const auto *Enum : ED->enumerators()) {
-Enumerators.push_back(DBuilder.createEnumerator(
-Enum->getName(), Enum->getInitVal().getSExtValue()));
+const auto &InitVal = Enum->getInitVal();
+auto Value = IsSigned ? InitVal.getSExtValue() : InitVal.getZExtValue();
+Enumerators.push_back(
+DBuilder.createEnumerator(Enum->getName(), Value, !IsSigned));
   }
 
   // Return a CompositeType for the enum itself.
   llvm::DINodeArray EltArray = DBuilder.getOrCreateArray(Enumerators);
 
   llvm::DIFile *DefUnit = getOrCreateFile(ED->getLocation());
   unsigned Line = getLineNumber(ED->getLocation());
   llvm::DIScope *EnumContext = getDeclContextDescriptor(ED);
-  llvm::DIType *ClassTy =
-  ED->isFixed() ? getOrCreateType(ED->getIntegerType(), DefUnit) : nullptr;
+  llvm::DIType *ClassTy = getOrCreateType(ED->getIntegerType(), DefUnit);
   return DBuilder.createEnumerationType(EnumContext, ED->getName(), DefUnit,
 Line, Size, Align, EltArray, ClassTy,
-FullName);
+FullName, ED->isFixed());
 }
 
 llvm::DIMacro *CGDebugInfo::CreateMacro(llvm::DIMacroFile *Parent,
Index: test/Modules/ModuleDebugInfo.cpp
===
--- test/Modules/ModuleDebugInfo.cpp
+++ test/Modules/ModuleDebugInfo.cpp
@@ -48,7 +48,7 @@
 // CHECK: !DICompositeType(tag: DW_TAG_enumeration_type,
 // CHECK-NOT:  name:
 // CHECK-SAME: )
-// CHECK: !DIEnumerator(name: "e5", value: 5)
+// CHECK: !DIEnumerator(name: "e5", value: 5, isUnsigned: true)
 
 // CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
 // no mangled name here yet.
Index: test/CodeGen/debug-info-enum.cpp
===
--- test/CodeGen/debug-info-enum.cpp
+++ test/CodeGen/debug-info-enum.cpp
@@ -0,0 +1,100 @@
+// Test enumeration representation in debuig info metadata:
+// * test value representation for each possible underlying integer type
+// * test the integer type is as expected
+// * test the DW_AT_enum_class attribute is present (resp. absent) as expected.
+
+// RUN: %clang -target x86_64-linux -g -S -emit-llvm -o - %s | FileCheck %s
+
+
+enum class E0 : signed char {
+  A0 = -128,
+  B0 = 127,
+} x0;
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E0"
+// CHECK-SAME: baseType: ![[SCHAR:[0-9]+]]
+// CHECK-SAME: DIFlagFixedEnum
+// CHECK-SAME: elements: ![[ELTS0:[0-9]+]]
+// CHECK: ![[SCHAR]] = !DIBasicType(name: "signed char", size: 8, encoding: DW_ATE_signed_char)
+// CHECK: ![[ELTS0]] = !{![[A0:[0-9]+]], ![[B0:[0-9]+]]}
+// CHECK: ![[A0]] = !DIEnumerator(name: "A0", value: -128)
+// CHECK: ![[B0]] = !DIEnumerator(name: "B0", value: 127)
+
+enum class E1 : unsigned char { A1 = 255 } x1;
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E1"
+// CHECK-SAME: baseType: ![[UCHAR:[0-9]+]]
+// CHECK-SAME: DIFlagFixedEnum
+// CHECK-SAME: elements: ![[ELTS1:[0-9]+]]
+// CHECK: ![[UCHAR]] = !DIBasicType(name: "unsigned char", size: 8, encoding: DW_ATE_unsigned_char)
+// CHECK: ![[ELTS1]] = !{![[A1:[0-9]+]]}
+// CHECK: ![[A1]] = !DIEnumerator(name: "A1", value: 255, isUnsigned: true)
+
+enum class E2 : signed short {
+  A2 = -32768,
+  B2 = 32767,
+} x2;
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E2"
+// CHECK-SAME: baseType: ![[SHORT:[0-9]+]]
+// CHECK-SAME: DIFlagFixedEnum
+// CHECK-SAME: elements: ![[ELTS2:[0-9]+]]
+// CHECK: ![[SHORT]] = !DIBasicType(name: "short", size: 16, encoding: DW_ATE_signed)
+// CHECK: ![[ELTS2]] = !{![[A2:[0-9]+]], ![[B2:[0-9]+]]}
+// CHECK: ![[A2]] = !DIEnumerator(name: "A2", value: -32768)
+// CHECK: ![[B2]] = !DIEnumerator(name: "B2", value: 32767)
+
+enum class E3 : unsigned short { A3 = 65535 } x3;
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E3"
+// CHECK-SAME: baseType: ![[USHORT:[0-9]+]]
+// CHECK-SAME: DIFlagFixedEnum
+// CHECK-SAME: elements: ![[ELTS3:[0-9]+]]
+// CHECK: ![[USHORT]] = !DIBasicType(name: "unsigned short", size: 16, encoding: DW_ATE_unsigned)
+// CHECK: ![[ELTS3]] = !{![[A

[PATCH] D33676: Place implictly declared functions at block scope

[Momchil Velikov via Phabricator via cfe-commits]

chill added a subscriber: cfe-commits.
chill added a comment.

This issue https://bugs.llvm.org//show_bug.cgi?id=2266 does not appear to 
trigger anymore, with this patch applied.


https://reviews.llvm.org/D33676



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

[PATCH] D33676: Place implictly declared functions at block scope

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In https://reviews.llvm.org/D33676#767746, @rogfer01 wrote:

> I understand that the scope `S` in this case is a (function) prototype scope 
> so it would not be the innermost block scope, would it? That said, GCC does 
> not accept `p_ok` above so probably this behaviour is sensible, after all in 
> C99 this is an extension.


I actually missed the possibility of a function call expression to appear in 
such a context. I also think that GCC (and Clang) should accept the `p_ok` 
initializer, as they do if one adds `extern int call_to_undeclared();` at the 
beginning of the block.


https://reviews.llvm.org/D33676



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

[PATCH] D33676: Place implictly declared functions at block scope

[Momchil Velikov via Phabricator via cfe-commits]

chill updated this revision to Diff 101522.
chill added a comment.

Update 1 notes.

The scopes for compound statements are explicitly marked as such, so then 
`Sema::ImplicitylDefineFunction` can ascend to the nearest enclosing scope. The 
function scopes (ones with `Scope:FnScope`) do not necessarily have the 
`Scope::CompoundStmtScope` flag set, that's why this flag is tested separately. 
I have considered, for consistency, setting the compound statement flag to all 
kinds of compound statement scopes, including function scopes, try/catch/SEH, 
block scopes, etc., but , given that this makes difference just for C90, I 
decided that such sweeping changes are unwarranted.


https://reviews.llvm.org/D33676

Files:
  include/clang/Sema/Scope.h
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaDecl.cpp
  test/Sema/implicit-decl-c90.c
  test/Sema/implicit-decl.c

Index: test/Sema/implicit-decl.c
===
--- test/Sema/implicit-decl.c
+++ test/Sema/implicit-decl.c
@@ -9,16 +9,15 @@
int32_t *vector[16];
const char compDesc[16 + 1];
int32_t compCount = 0;
-   if (_CFCalendarDecomposeAbsoluteTimeV(compDesc, vector, compCount)) { // expected-note {{previous implicit declaration is here}} \
- expected-error {{implicit declaration of function '_CFCalendarDecomposeAbsoluteTimeV' is invalid in C99}}
+   if (_CFCalendarDecomposeAbsoluteTimeV(compDesc, vector, compCount)) { // expected-error {{implicit declaration of function '_CFCalendarDecomposeAbsoluteTimeV' is invalid in C99}}
}
 
printg("Hello, World!\n"); // expected-error{{implicit declaration of function 'printg' is invalid in C99}} \
   // expected-note{{did you mean 'printf'?}}
 
   __builtin_is_les(1, 3); // expected-error{{use of unknown builtin '__builtin_is_les'}}
 }
-Boolean _CFCalendarDecomposeAbsoluteTimeV(const char *componentDesc, int32_t **vector, int32_t count) { // expected-error{{conflicting types for '_CFCalendarDecomposeAbsoluteTimeV'}}
+Boolean _CFCalendarDecomposeAbsoluteTimeV(const char *componentDesc, int32_t **vector, int32_t count) {
  return 0;
 }
 
Index: test/Sema/implicit-decl-c90.c
===
--- /dev/null
+++ test/Sema/implicit-decl-c90.c
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 %s -std=c90 -verify -fsyntax-only
+void t0(int x) {
+  int (*p)();
+  if(x > 0)
+x = g() + 1;
+  p = g;
+  if(x < 0) {
+extern void u(int (*)[h()]);
+int (*q)() = h;
+  }
+  p = h; /* expected-error {{use of undeclared identifier 'h'}} */
+}
+
+void t1(int x) {
+  int (*p)();
+  switch (x) {
+g();
+  case 0:
+x = h() + 1;
+break;
+  case 1:
+p = g;
+p = h;
+break;
+  }
+  p = g; /* expected-error {{use of undeclared identifier 'g'}} */
+  p = h; /* expected-error {{use of undeclared identifier 'h'}} */
+}
+
+int (*p)() = g; /* expected-error {{use of undeclared identifier 'g'}} */
+int (*q)() = h; /* expected-error {{use of undeclared identifier 'h'}} */
+
+float g(); /* not expecting conflicting types diagnostics here */
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -12605,12 +12605,16 @@
 SourceLocation());
   D.SetIdentifier(&II, Loc);
 
-  // Insert this function into translation-unit scope.
+  // Insert this function into the enclosing block scope.
+  while (S && !S->isCompoundStmtScope() && !S->isFunctionScope())
+S = S->getParent();
+  if (S == nullptr)
+S = TUScope;
 
   DeclContext *PrevDC = CurContext;
   CurContext = Context.getTranslationUnitDecl();
 
-  FunctionDecl *FD = cast(ActOnDeclarator(TUScope, D));
+  FunctionDecl *FD = cast(ActOnDeclarator(S, D));
   FD->setImplicit();
 
   CurContext = PrevDC;
Index: lib/Parse/ParseStmt.cpp
===
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -840,7 +840,8 @@
 }
 
 StmtResult Parser::ParseCompoundStatement(bool isStmtExpr) {
-  return ParseCompoundStatement(isStmtExpr, Scope::DeclScope);
+  return ParseCompoundStatement(isStmtExpr,
+Scope::DeclScope | Scope::CompoundStmtScope);
 }
 
 /// ParseCompoundStatement - Parse a "{}" block.
Index: include/clang/Sema/Scope.h
===
--- include/clang/Sema/Scope.h
+++ include/clang/Sema/Scope.h
@@ -124,6 +124,9 @@
 
 /// We are currently in the filter expression of an SEH except block.
 SEHFilterScope = 0x20,
+
+/// This is a compound statement scope.
+CompoundStmtScope = 0x40,
   };
 private:
   /// The parent scope for this scope.  This is null for the translation-unit
@@ -429,6 +432,9 @@
   /// \brief Determine whether this scope is a SEH '__except' block.
   bool isSEHExceptScope() const { return getFlags() & Scope::SEHExceptScope; }
 
+  /// \brief Deter

[PATCH] D33676: Place implictly declared functions at block scope

[Momchil Velikov via Phabricator via cfe-commits]

chill updated this revision to Diff 102802.
chill added a comment.

Update 2: added a testcase involving expression statements


https://reviews.llvm.org/D33676

Files:
  include/clang/Sema/Scope.h
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaDecl.cpp
  test/Sema/implicit-decl-c90.c
  test/Sema/implicit-decl.c

Index: test/Sema/implicit-decl.c
===
--- test/Sema/implicit-decl.c
+++ test/Sema/implicit-decl.c
@@ -9,16 +9,15 @@
int32_t *vector[16];
const char compDesc[16 + 1];
int32_t compCount = 0;
-   if (_CFCalendarDecomposeAbsoluteTimeV(compDesc, vector, compCount)) { // expected-note {{previous implicit declaration is here}} \
- expected-error {{implicit declaration of function '_CFCalendarDecomposeAbsoluteTimeV' is invalid in C99}}
+   if (_CFCalendarDecomposeAbsoluteTimeV(compDesc, vector, compCount)) { // expected-error {{implicit declaration of function '_CFCalendarDecomposeAbsoluteTimeV' is invalid in C99}}
}
 
printg("Hello, World!\n"); // expected-error{{implicit declaration of function 'printg' is invalid in C99}} \
   // expected-note{{did you mean 'printf'?}}
 
   __builtin_is_les(1, 3); // expected-error{{use of unknown builtin '__builtin_is_les'}}
 }
-Boolean _CFCalendarDecomposeAbsoluteTimeV(const char *componentDesc, int32_t **vector, int32_t count) { // expected-error{{conflicting types for '_CFCalendarDecomposeAbsoluteTimeV'}}
+Boolean _CFCalendarDecomposeAbsoluteTimeV(const char *componentDesc, int32_t **vector, int32_t count) {
  return 0;
 }
 
Index: test/Sema/implicit-decl-c90.c
===
--- /dev/null
+++ test/Sema/implicit-decl-c90.c
@@ -0,0 +1,39 @@
+// RUN: %clang_cc1 %s -std=c90 -verify -fsyntax-only
+void t0(int x) {
+  int (*p)();
+  if(x > 0)
+x = g() + 1;
+  p = g;
+  if(x < 0) {
+extern void u(int (*)[h()]);
+int (*q)() = h;
+  }
+  p = h; /* expected-error {{use of undeclared identifier 'h'}} */
+}
+
+void t1(int x) {
+  int (*p)();
+  switch (x) {
+g();
+  case 0:
+x = h() + 1;
+break;
+  case 1:
+p = g;
+p = h;
+break;
+  }
+  p = g; /* expected-error {{use of undeclared identifier 'g'}} */
+  p = h; /* expected-error {{use of undeclared identifier 'h'}} */
+}
+
+int t2(int x) {
+  int y = ({ if (x > 0) x = g() + 1; 2*x; });
+  int (*p)() = g; /* expected-error {{use of undeclared identifier 'g'}} */
+  return y;
+}
+
+int (*p)() = g; /* expected-error {{use of undeclared identifier 'g'}} */
+int (*q)() = h; /* expected-error {{use of undeclared identifier 'h'}} */
+
+float g(); /* not expecting conflicting types diagnostics here */
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -12605,12 +12605,16 @@
 SourceLocation());
   D.SetIdentifier(&II, Loc);
 
-  // Insert this function into translation-unit scope.
+  // Insert this function into the enclosing block scope.
+  while (S && !S->isCompoundStmtScope() && !S->isFunctionScope())
+S = S->getParent();
+  if (S == nullptr)
+S = TUScope;
 
   DeclContext *PrevDC = CurContext;
   CurContext = Context.getTranslationUnitDecl();
 
-  FunctionDecl *FD = cast(ActOnDeclarator(TUScope, D));
+  FunctionDecl *FD = cast(ActOnDeclarator(S, D));
   FD->setImplicit();
 
   CurContext = PrevDC;
Index: lib/Parse/ParseStmt.cpp
===
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -840,7 +840,8 @@
 }
 
 StmtResult Parser::ParseCompoundStatement(bool isStmtExpr) {
-  return ParseCompoundStatement(isStmtExpr, Scope::DeclScope);
+  return ParseCompoundStatement(isStmtExpr,
+Scope::DeclScope | Scope::CompoundStmtScope);
 }
 
 /// ParseCompoundStatement - Parse a "{}" block.
Index: include/clang/Sema/Scope.h
===
--- include/clang/Sema/Scope.h
+++ include/clang/Sema/Scope.h
@@ -124,6 +124,9 @@
 
 /// We are currently in the filter expression of an SEH except block.
 SEHFilterScope = 0x20,
+
+/// This is a compound statement scope.
+CompoundStmtScope = 0x40,
   };
 private:
   /// The parent scope for this scope.  This is null for the translation-unit
@@ -429,6 +432,9 @@
   /// \brief Determine whether this scope is a SEH '__except' block.
   bool isSEHExceptScope() const { return getFlags() & Scope::SEHExceptScope; }
 
+  /// \brief Determine whether this scope is a compound statement scope.
+  bool isCompoundStmtScope() const { return getFlags() & Scope::CompoundStmtScope; }
+
   /// \brief Returns if rhs has a higher scope depth than this.
   ///
   /// The caller is responsible for calling this only if one of the two scopes
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
h

[PATCH] D33676: Place implictly declared functions at block scope

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: test/Sema/implicit-decl.c:12
int32_t compCount = 0;
-   if (_CFCalendarDecomposeAbsoluteTimeV(compDesc, vector, compCount)) { // 
expected-note {{previous implicit declaration is here}} \
- expected-error {{implicit declaration of function 
'_CFCalendarDecomposeAbsoluteTimeV' is invalid in C99}}
+   if (_CFCalendarDecomposeAbsoluteTimeV(compDesc, vector, compCount)) { // 
expected-error {{implicit declaration of function 
'_CFCalendarDecomposeAbsoluteTimeV' is invalid in C99}}
}

rogfer01 wrote:
> Is there a way not to lose the note?
The note is a part of the conflicting types diagnostic, that's no longer issued.


https://reviews.llvm.org/D33676



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

[PATCH] D33676: Place implictly declared functions at block scope

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Ping?


https://reviews.llvm.org/D33676



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

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill updated this revision to Diff 149252.
chill added a comment.

Update:

- similar changes needed for AArch64
- added/updated tests


https://reviews.llvm.org/D46013

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/RecordLayout.h
  lib/AST/ASTContext.cpp
  lib/AST/RecordLayout.cpp
  lib/AST/RecordLayoutBuilder.cpp
  lib/CodeGen/TargetInfo.cpp
  test/CodeGen/aapcs-align.cc
  test/CodeGen/aapcs64-align.cc
  test/CodeGen/arm-arguments.c
  test/CodeGen/arm64-arguments.c

Index: test/CodeGen/arm64-arguments.c
===
--- test/CodeGen/arm64-arguments.c
+++ test/CodeGen/arm64-arguments.c
@@ -216,11 +216,11 @@
 
 typedef __attribute__((neon_vector_type(4))) int int32x4_t;
 int32x4_t f36(int i, s36_with_align s1, s36_with_align s2) {
-// CHECK: define <4 x i32> @f36(i32 %i, i128 %s1.coerce, i128 %s2.coerce)
+// CHECK: define <4 x i32> @f36(i32 %i, [2 x i64] %s1.coerce, [2 x i64] %s2.coerce)
 // CHECK: %s1 = alloca %struct.s36, align 16
 // CHECK: %s2 = alloca %struct.s36, align 16
-// CHECK: store i128 %s1.coerce, i128* %{{.*}}, align 16
-// CHECK: store i128 %s2.coerce, i128* %{{.*}}, align 16
+// CHECK: store [2 x i64] %s1.coerce, [2 x i64]* %{{.*}}, align 16
+// CHECK: store [2 x i64] %s2.coerce, [2 x i64]* %{{.*}}, align 16
 // CHECK: %[[a:.*]] = bitcast %struct.s36* %s1 to <4 x i32>*
 // CHECK: load <4 x i32>, <4 x i32>* %[[a]], align 16
 // CHECK: %[[b:.*]] = bitcast %struct.s36* %s2 to <4 x i32>*
@@ -325,11 +325,11 @@
 // passing aligned structs in registers
 __attribute__ ((noinline))
 int f39(int i, s39_with_align s1, s39_with_align s2) {
-// CHECK: define i32 @f39(i32 %i, i128 %s1.coerce, i128 %s2.coerce)
+// CHECK: define i32 @f39(i32 %i, [2 x i64] %s1.coerce, [2 x i64] %s2.coerce)
 // CHECK: %s1 = alloca %struct.s39, align 16
 // CHECK: %s2 = alloca %struct.s39, align 16
-// CHECK: store i128 %s1.coerce, i128* %{{.*}}, align 16
-// CHECK: store i128 %s2.coerce, i128* %{{.*}}, align 16
+// CHECK: store [2 x i64] %s1.coerce, [2 x i64]* %{{.*}}, align 16
+// CHECK: store [2 x i64] %s2.coerce, [2 x i64]* %{{.*}}, align 16
 // CHECK: getelementptr inbounds %struct.s39, %struct.s39* %s1, i32 0, i32 0
 // CHECK: getelementptr inbounds %struct.s39, %struct.s39* %s2, i32 0, i32 0
 // CHECK: getelementptr inbounds %struct.s39, %struct.s39* %s1, i32 0, i32 1
@@ -340,31 +340,31 @@
 s39_with_align g39_2;
 int caller39() {
 // CHECK: define i32 @caller39()
-// CHECK: %[[a:.*]] = load i128, i128* bitcast (%struct.s39* @g39 to i128*), align 16
-// CHECK: %[[b:.*]] = load i128, i128* bitcast (%struct.s39* @g39_2 to i128*), align 16
-// CHECK: call i32 @f39(i32 3, i128 %[[a]], i128 %[[b]])
+// CHECK: %[[a:.*]] = load [2 x i64], [2 x i64]* bitcast (%struct.s39* @g39 to [2 x i64]*), align 16
+// CHECK: %[[b:.*]] = load [2 x i64], [2 x i64]* bitcast (%struct.s39* @g39_2 to [2 x i64]*), align 16
+// CHECK: call i32 @f39(i32 3, [2 x i64] %[[a]], [2 x i64] %[[b]])
   return f39(3, g39, g39_2);
 }
 // passing aligned structs on stack
 __attribute__ ((noinline))
 int f39_stack(int i, int i2, int i3, int i4, int i5, int i6, int i7, int i8,
   int i9, s39_with_align s1, s39_with_align s2) {
-// CHECK: define i32 @f39_stack(i32 %i, i32 %i2, i32 %i3, i32 %i4, i32 %i5, i32 %i6, i32 %i7, i32 %i8, i32 %i9, i128 %s1.coerce, i128 %s2.coerce)
+// CHECK  define i32 @f39_stack(i32 %i, i32 %i2, i32 %i3, i32 %i4, i32 %i5, i32 %i6, i32 %i7, i32 %i8, i32 %i9, [2 x i64] %s1.coerce, [2 x i64] %s2.coerce)
 // CHECK: %s1 = alloca %struct.s39, align 16
 // CHECK: %s2 = alloca %struct.s39, align 16
-// CHECK: store i128 %s1.coerce, i128* %{{.*}}, align 16
-// CHECK: store i128 %s2.coerce, i128* %{{.*}}, align 16
+// CHECK: store [2 x i64] %s1.coerce, [2 x i64]* %{{.*}}, align 16
+// CHECK: store [2 x i64] %s2.coerce, [2 x i64]* %{{.*}}, align 16
 // CHECK: getelementptr inbounds %struct.s39, %struct.s39* %s1, i32 0, i32 0
 // CHECK: getelementptr inbounds %struct.s39, %struct.s39* %s2, i32 0, i32 0
 // CHECK: getelementptr inbounds %struct.s39, %struct.s39* %s1, i32 0, i32 1
 // CHECK: getelementptr inbounds %struct.s39, %struct.s39* %s2, i32 0, i32 1
   return s1.i + s2.i + i + i2 + i3 + i4 + i5 + i6 + i7 + i8 + i9 + s1.s + s2.s;
 }
 int caller39_stack() {
 // CHECK: define i32 @caller39_stack()
-// CHECK: %[[a:.*]] = load i128, i128* bitcast (%struct.s39* @g39 to i128*), align 16
-// CHECK: %[[b:.*]] = load i128, i128* bitcast (%struct.s39* @g39_2 to i128*), align 16
-// CHECK: call i32 @f39_stack(i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7, i32 8, i32 9, i128 %[[a]], i128 %[[b]])
+// CHECK: %[[a:.*]] = load [2 x i64], [2 x i64]* bitcast (%struct.s39* @g39 to [2 x i64]*), align 16
+// CHECK: %[[b:.*]] = load [2 x i64], [2 x i64]* bitcast (%struct.s39* @g39_2 to [2 x i64]*), align 16
+// CHECK: call i32 @f39_stack(i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7, i32 8, i32 9, [2 x i64] %[[a]], [2 x i64] %[[b]])
   return f39_stack(1, 2, 3, 4, 5, 6, 7, 8, 9, g

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill updated this revision to Diff 149536.
chill added a comment.

Update:

- fix only APCS, don't touch other ABIs
- misc other


https://reviews.llvm.org/D46013

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/RecordLayout.h
  lib/AST/ASTContext.cpp
  lib/AST/RecordLayout.cpp
  lib/AST/RecordLayoutBuilder.cpp
  lib/CodeGen/TargetInfo.cpp
  test/CodeGen/aapcs-align.cc
  test/CodeGen/aapcs64-align.cc
  test/CodeGen/arm-arguments.c

Index: test/CodeGen/arm-arguments.c
===
--- test/CodeGen/arm-arguments.c
+++ test/CodeGen/arm-arguments.c
@@ -211,10 +211,13 @@
 // APCS-GNU: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align {{[0-9]+}} %[[b]], i8* align {{[0-9]+}} %[[c]]
 // APCS-GNU: %[[d:.*]] = bitcast %struct.s35* %[[a]] to <4 x float>*
 // APCS-GNU: load <4 x float>, <4 x float>* %[[d]], align 16
-// AAPCS-LABEL: define arm_aapcscc <4 x float> @f35(i32 %i, %struct.s35* byval align 8, %struct.s35* byval align 8)
-// AAPCS: %[[a:.*]] = alloca %struct.s35, align 16
-// AAPCS: %[[b:.*]] = bitcast %struct.s35* %[[a]] to i8*
-// AAPCS: %[[c:.*]] = bitcast %struct.s35* %0 to i8*
-// AAPCS: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 16 %[[b]], i8* align 8 %[[c]]
-// AAPCS: %[[d:.*]] = bitcast %struct.s35* %[[a]] to <4 x float>*
-// AAPCS: load <4 x float>, <4 x float>* %[[d]], align 16
+
+// AAPCS-LABEL: define arm_aapcscc <4 x float> @f35(i32 %i, %struct.s35* byval align 4 %s1, %struct.s35* byval align 4 %s2)
+// AAPCS: %[[a_addr:.*]] = alloca <4 x float>, align 16
+// AAPCS: %[[b_addr:.*]] = alloca <4 x float>, align 16
+// AAPCS: %[[p1:.*]] = bitcast %struct.s35* %s1 to <4 x float>*
+// AAPCS: %[[a:.*]] = load <4 x float>, <4 x float>* %[[p1]], align 4
+// AAPCS: %[[p2:.*]] = bitcast %struct.s35* %s2 to <4 x float>*
+// AAPCS: %[[b:.*]] = load <4 x float>, <4 x float>* %[[p2]], align 4
+// AAPCS: store <4 x float> %[[a]], <4 x float>* %[[a_addr]], align 16
+// AAPCS: store <4 x float> %[[b]], <4 x float>* %[[b_addr]], align 16
Index: test/CodeGen/aapcs64-align.cc
===
--- /dev/null
+++ test/CodeGen/aapcs64-align.cc
@@ -0,0 +1,103 @@
+// REQUIRES: arm-registered-target
+// RUN: %clang_cc1 -triple aarch64-none-none-eabi \
+// RUN:   -O2 \
+// RUN:   -emit-llvm -o - %s | FileCheck %s
+
+extern "C" {
+
+// Base case, nothing interesting.
+struct S {
+  long x, y;
+};
+
+void f0(long, S);
+void f0m(long, long, long, long, long, S);
+void g0() {
+  S s = {6, 7};
+  f0(1, s);
+  f0m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g0
+// CHECK: call void @f0(i64 1, [2 x i64] [i64 6, i64 7]
+// CHECK: call void @f0m{{.*}}[2 x i64] [i64 6, i64 7]
+// CHECK: declare void @f0(i64, [2 x i64])
+// CHECK: declare void @f0m(i64, i64, i64, i64, i64, [2 x i64])
+
+// Aligned struct, passed according to its natural alignment.
+struct __attribute__((aligned(16))) S16 {
+  long x, y;
+} s16;
+
+void f1(long, S16);
+void f1m(long, long, long, long, long, S16);
+void g1() {
+  S16 s = {6, 7};
+  f1(1, s);
+  f1m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g1
+// CHECK: call void @f1{{.*}}[2 x i64] [i64 6, i64 7]
+// CHECK: call void @f1m{{.*}}[2 x i64] [i64 6, i64 7]
+// CHECK: declare void @f1(i64, [2 x i64])
+// CHECK: declare void @f1m(i64, i64, i64, i64, i64, [2 x i64])
+
+// Increased natural alignment.
+struct SF16 {
+  long x __attribute__((aligned(16)));
+  long y;
+};
+
+void f3(long, SF16);
+void f3m(long, long, long, long, long, SF16);
+void g3() {
+  SF16 s = {6, 7};
+  f3(1, s);
+  f3m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g3
+// CHECK: call void @f3(i64 1, i128 129127208515966861318)
+// CHECK: call void @f3m(i64 1, i64 2, i64 3, i64 4, i64 5, i128 129127208515966861318)
+// CHECK: declare void @f3(i64, i128)
+// CHECK: declare void @f3m(i64, i64, i64, i64, i64, i128)
+
+
+// Packed structure.
+struct  __attribute__((packed)) P {
+  int x;
+  long u;
+};
+
+void f4(int, P);
+void f4m(int, int, int, int, int, P);
+void g4() {
+  P s = {6, 7};
+  f4(1, s);
+  f4m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g4()
+// CHECK: call void @f4(i32 1, [2 x i64] [i64 30064771078, i64 0])
+// CHECK: void @f4m(i32 1, i32 2, i32 3, i32 4, i32 5, [2 x i64] [i64 30064771078, i64 0])
+// CHECK: declare void @f4(i32, [2 x i64])
+// CHECK: declare void @f4m(i32, i32, i32, i32, i32, [2 x i64])
+
+
+// Packed structure, overaligned, same as above.
+struct  __attribute__((packed, aligned(16))) P16 {
+  int x;
+  long y;
+};
+
+void f5(int, P16);
+void f5m(int, int, int, int, int, P16);
+  void g5() {
+P16 s = {6, 7};
+f5(1, s);
+f5m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g5()
+// CHECK: call void @f5(i32 1, [2 x i64] [i64 30064771078, i64 0])
+// CHECK: void @f5m(i32 1, i32 2, i32 3, i32 4, i32 5, [2 x i64] [i64 30064771078, i64 0])
+// CHECK: declare void @f5(i32, [2 x i64])
+// CHECK: declare void @f5m(i32, i32, i32, i32, i32, [2 x i64])
+
+}
Index: test/CodeGen/aapcs-align.

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill marked 2 inline comments as done.
chill added a comment.

In https://reviews.llvm.org/D46013#1118014, @efriedma wrote:

> I'm not sure Apple will want to mess with their ABI like this... adding some 
> reviewers.


Fair enough, I'd rather not touch it :)


https://reviews.llvm.org/D46013



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

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill updated this revision to Diff 150148.
chill added a comment.

Update: refactor a bit to not impose size overhead on targets, which don't use 
natural alignment.


https://reviews.llvm.org/D46013

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/RecordLayout.h
  lib/AST/ASTContext.cpp
  lib/AST/RecordLayout.cpp
  lib/AST/RecordLayoutBuilder.cpp
  lib/CodeGen/TargetInfo.cpp
  test/CodeGen/aapcs-align.cc
  test/CodeGen/aapcs64-align.cc
  test/CodeGen/arm-arguments.c

Index: test/CodeGen/arm-arguments.c
===
--- test/CodeGen/arm-arguments.c
+++ test/CodeGen/arm-arguments.c
@@ -211,10 +211,13 @@
 // APCS-GNU: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align {{[0-9]+}} %[[b]], i8* align {{[0-9]+}} %[[c]]
 // APCS-GNU: %[[d:.*]] = bitcast %struct.s35* %[[a]] to <4 x float>*
 // APCS-GNU: load <4 x float>, <4 x float>* %[[d]], align 16
-// AAPCS-LABEL: define arm_aapcscc <4 x float> @f35(i32 %i, %struct.s35* byval align 8, %struct.s35* byval align 8)
-// AAPCS: %[[a:.*]] = alloca %struct.s35, align 16
-// AAPCS: %[[b:.*]] = bitcast %struct.s35* %[[a]] to i8*
-// AAPCS: %[[c:.*]] = bitcast %struct.s35* %0 to i8*
-// AAPCS: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 16 %[[b]], i8* align 8 %[[c]]
-// AAPCS: %[[d:.*]] = bitcast %struct.s35* %[[a]] to <4 x float>*
-// AAPCS: load <4 x float>, <4 x float>* %[[d]], align 16
+
+// AAPCS-LABEL: define arm_aapcscc <4 x float> @f35(i32 %i, %struct.s35* byval align 4 %s1, %struct.s35* byval align 4 %s2)
+// AAPCS: %[[a_addr:.*]] = alloca <4 x float>, align 16
+// AAPCS: %[[b_addr:.*]] = alloca <4 x float>, align 16
+// AAPCS: %[[p1:.*]] = bitcast %struct.s35* %s1 to <4 x float>*
+// AAPCS: %[[a:.*]] = load <4 x float>, <4 x float>* %[[p1]], align 4
+// AAPCS: %[[p2:.*]] = bitcast %struct.s35* %s2 to <4 x float>*
+// AAPCS: %[[b:.*]] = load <4 x float>, <4 x float>* %[[p2]], align 4
+// AAPCS: store <4 x float> %[[a]], <4 x float>* %[[a_addr]], align 16
+// AAPCS: store <4 x float> %[[b]], <4 x float>* %[[b_addr]], align 16
Index: test/CodeGen/aapcs64-align.cc
===
--- /dev/null
+++ test/CodeGen/aapcs64-align.cc
@@ -0,0 +1,103 @@
+// REQUIRES: arm-registered-target
+// RUN: %clang_cc1 -triple aarch64-none-none-eabi \
+// RUN:   -O2 \
+// RUN:   -emit-llvm -o - %s | FileCheck %s
+
+extern "C" {
+
+// Base case, nothing interesting.
+struct S {
+  long x, y;
+};
+
+void f0(long, S);
+void f0m(long, long, long, long, long, S);
+void g0() {
+  S s = {6, 7};
+  f0(1, s);
+  f0m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g0
+// CHECK: call void @f0(i64 1, [2 x i64] [i64 6, i64 7]
+// CHECK: call void @f0m{{.*}}[2 x i64] [i64 6, i64 7]
+// CHECK: declare void @f0(i64, [2 x i64])
+// CHECK: declare void @f0m(i64, i64, i64, i64, i64, [2 x i64])
+
+// Aligned struct, passed according to its natural alignment.
+struct __attribute__((aligned(16))) S16 {
+  long x, y;
+} s16;
+
+void f1(long, S16);
+void f1m(long, long, long, long, long, S16);
+void g1() {
+  S16 s = {6, 7};
+  f1(1, s);
+  f1m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g1
+// CHECK: call void @f1{{.*}}[2 x i64] [i64 6, i64 7]
+// CHECK: call void @f1m{{.*}}[2 x i64] [i64 6, i64 7]
+// CHECK: declare void @f1(i64, [2 x i64])
+// CHECK: declare void @f1m(i64, i64, i64, i64, i64, [2 x i64])
+
+// Increased natural alignment.
+struct SF16 {
+  long x __attribute__((aligned(16)));
+  long y;
+};
+
+void f3(long, SF16);
+void f3m(long, long, long, long, long, SF16);
+void g3() {
+  SF16 s = {6, 7};
+  f3(1, s);
+  f3m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g3
+// CHECK: call void @f3(i64 1, i128 129127208515966861318)
+// CHECK: call void @f3m(i64 1, i64 2, i64 3, i64 4, i64 5, i128 129127208515966861318)
+// CHECK: declare void @f3(i64, i128)
+// CHECK: declare void @f3m(i64, i64, i64, i64, i64, i128)
+
+
+// Packed structure.
+struct  __attribute__((packed)) P {
+  int x;
+  long u;
+};
+
+void f4(int, P);
+void f4m(int, int, int, int, int, P);
+void g4() {
+  P s = {6, 7};
+  f4(1, s);
+  f4m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g4()
+// CHECK: call void @f4(i32 1, [2 x i64] [i64 30064771078, i64 0])
+// CHECK: void @f4m(i32 1, i32 2, i32 3, i32 4, i32 5, [2 x i64] [i64 30064771078, i64 0])
+// CHECK: declare void @f4(i32, [2 x i64])
+// CHECK: declare void @f4m(i32, i32, i32, i32, i32, [2 x i64])
+
+
+// Packed structure, overaligned, same as above.
+struct  __attribute__((packed, aligned(16))) P16 {
+  int x;
+  long y;
+};
+
+void f5(int, P16);
+void f5m(int, int, int, int, int, P16);
+  void g5() {
+P16 s = {6, 7};
+f5(1, s);
+f5m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g5()
+// CHECK: call void @f5(i32 1, [2 x i64] [i64 30064771078, i64 0])
+// CHECK: void @f5m(i32 1, i32 2, i32 3, i32 4, i32 5, [2 x i64] [i64 30064771078, i64 0])
+// CHECK: declare void @f5(i32, [2 x i64])
+// CHECK: declare void @f5m(i32, i32, i32, i32, i32, [2 x i64])

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Ping?


https://reviews.llvm.org/D46013



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

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Ping?


https://reviews.llvm.org/D46013



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

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In https://reviews.llvm.org/D46013#1140336, @t.p.northover wrote:

> I'm fine with the ABI changes, but I'm not very convinced by the 
> "NaturalAlignment" name.


I'd rather not put target names in API functions. The meaning of that field is 
pretty target
independent ("alignment of type, before alignment adjustments")
If we use it only in ARM/AArch64 we can put a comment in that sense and
maybe rename it to `UnadjustedAlignement` or something else more descriptive 
than `NaturalAlignement` ?


https://reviews.llvm.org/D46013



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

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: lib/CodeGen/TargetInfo.cpp:5055
+  Alignment = getContext().getTypeNaturalAlign(Ty);
+  Alignment = std::min(std::max(Alignment, 64u), 128u);
+} else {

t.p.northover wrote:
> I think the max/min logic is more confusing here than the alternative:
> 
> Alignment = Alignment < 128 ? 64 : 128;
I'll change it.


https://reviews.llvm.org/D46013



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

[PATCH] D64415: Consistent types and naming for AArch64 target features (NFC)

[Momchil Velikov via Phabricator via cfe-commits]

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

Going to commit as obvious.


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

https://reviews.llvm.org/D64415



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

[PATCH] D64415: Consistent types and naming for AArch64 target features (NFC)

[Momchil Velikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL366315: [AArch64] Consistent types and naming for AArch64 
target features (NFC) (authored by chill, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64415?vs=210286&id=210290#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64415

Files:
  cfe/trunk/lib/Basic/Targets/AArch64.cpp
  cfe/trunk/lib/Basic/Targets/AArch64.h

Index: cfe/trunk/lib/Basic/Targets/AArch64.cpp
===
--- cfe/trunk/lib/Basic/Targets/AArch64.cpp
+++ cfe/trunk/lib/Basic/Targets/AArch64.cpp
@@ -199,13 +199,13 @@
   if (FPU & SveMode)
 Builder.defineMacro("__ARM_FEATURE_SVE", "1");
 
-  if (CRC)
+  if (HasCRC)
 Builder.defineMacro("__ARM_FEATURE_CRC32", "1");
 
-  if (Crypto)
+  if (HasCrypto)
 Builder.defineMacro("__ARM_FEATURE_CRYPTO", "1");
 
-  if (Unaligned)
+  if (HasUnaligned)
 Builder.defineMacro("__ARM_FEATURE_UNALIGNED", "1");
 
   if ((FPU & NeonMode) && HasFullFP16)
@@ -263,13 +263,13 @@
 bool AArch64TargetInfo::handleTargetFeatures(std::vector &Features,
  DiagnosticsEngine &Diags) {
   FPU = FPUMode;
-  CRC = 0;
-  Crypto = 0;
-  Unaligned = 1;
-  HasFullFP16 = 0;
-  HasDotProd = 0;
-  HasFP16FML = 0;
-  HasMTE = 0;
+  HasCRC = false;
+  HasCrypto = false;
+  HasUnaligned = true;
+  HasFullFP16 = false;
+  HasDotProd = false;
+  HasFP16FML = false;
+  HasMTE = false;
   ArchKind = llvm::AArch64::ArchKind::ARMV8A;
 
   for (const auto &Feature : Features) {
@@ -278,11 +278,11 @@
 if (Feature == "+sve")
   FPU |= SveMode;
 if (Feature == "+crc")
-  CRC = 1;
+  HasCRC = true;
 if (Feature == "+crypto")
-  Crypto = 1;
+  HasCrypto = true;
 if (Feature == "+strict-align")
-  Unaligned = 0;
+  HasUnaligned = false;
 if (Feature == "+v8.1a")
   ArchKind = llvm::AArch64::ArchKind::ARMV8_1A;
 if (Feature == "+v8.2a")
@@ -294,13 +294,13 @@
 if (Feature == "+v8.5a")
   ArchKind = llvm::AArch64::ArchKind::ARMV8_5A;
 if (Feature == "+fullfp16")
-  HasFullFP16 = 1;
+  HasFullFP16 = true;
 if (Feature == "+dotprod")
-  HasDotProd = 1;
+  HasDotProd = true;
 if (Feature == "+fp16fml")
-  HasFP16FML = 1;
+  HasFP16FML = true;
 if (Feature == "+mte")
-  HasMTE = 1;
+  HasMTE = true;
   }
 
   setDataLayout();
Index: cfe/trunk/lib/Basic/Targets/AArch64.h
===
--- cfe/trunk/lib/Basic/Targets/AArch64.h
+++ cfe/trunk/lib/Basic/Targets/AArch64.h
@@ -28,13 +28,14 @@
   enum FPUModeEnum { FPUMode, NeonMode = (1 << 0), SveMode = (1 << 1) };
 
   unsigned FPU;
-  unsigned CRC;
-  unsigned Crypto;
-  unsigned Unaligned;
-  unsigned HasFullFP16;
-  unsigned HasDotProd;
-  unsigned HasFP16FML;
-  unsigned HasMTE;
+  bool HasCRC;
+  bool HasCrypto;
+  bool HasUnaligned;
+  bool HasFullFP16;
+  bool HasDotProd;
+  bool HasFP16FML;
+  bool HasMTE;
+
   llvm::AArch64::ArchKind ArchKind;
 
   static const Builtin::Info BuiltinInfo[];
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64415: Consistent types and naming for AArch64 target features (NFC)

[Momchil Velikov via Phabricator via cfe-commits]

chill updated this revision to Diff 210286.

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

https://reviews.llvm.org/D64415

Files:
  clang/lib/Basic/Targets/AArch64.cpp
  clang/lib/Basic/Targets/AArch64.h

Index: clang/lib/Basic/Targets/AArch64.h
===
--- clang/lib/Basic/Targets/AArch64.h
+++ clang/lib/Basic/Targets/AArch64.h
@@ -28,13 +28,14 @@
   enum FPUModeEnum { FPUMode, NeonMode = (1 << 0), SveMode = (1 << 1) };
 
   unsigned FPU;
-  unsigned CRC;
-  unsigned Crypto;
-  unsigned Unaligned;
-  unsigned HasFullFP16;
-  unsigned HasDotProd;
-  unsigned HasFP16FML;
-  unsigned HasMTE;
+  bool HasCRC;
+  bool HasCrypto;
+  bool HasUnaligned;
+  bool HasFullFP16;
+  bool HasDotProd;
+  bool HasFP16FML;
+  bool HasMTE;
+
   llvm::AArch64::ArchKind ArchKind;
 
   static const Builtin::Info BuiltinInfo[];
Index: clang/lib/Basic/Targets/AArch64.cpp
===
--- clang/lib/Basic/Targets/AArch64.cpp
+++ clang/lib/Basic/Targets/AArch64.cpp
@@ -199,13 +199,13 @@
   if (FPU & SveMode)
 Builder.defineMacro("__ARM_FEATURE_SVE", "1");
 
-  if (CRC)
+  if (HasCRC)
 Builder.defineMacro("__ARM_FEATURE_CRC32", "1");
 
-  if (Crypto)
+  if (HasCrypto)
 Builder.defineMacro("__ARM_FEATURE_CRYPTO", "1");
 
-  if (Unaligned)
+  if (HasUnaligned)
 Builder.defineMacro("__ARM_FEATURE_UNALIGNED", "1");
 
   if ((FPU & NeonMode) && HasFullFP16)
@@ -263,13 +263,13 @@
 bool AArch64TargetInfo::handleTargetFeatures(std::vector &Features,
  DiagnosticsEngine &Diags) {
   FPU = FPUMode;
-  CRC = 0;
-  Crypto = 0;
-  Unaligned = 1;
-  HasFullFP16 = 0;
-  HasDotProd = 0;
-  HasFP16FML = 0;
-  HasMTE = 0;
+  HasCRC = false;
+  HasCrypto = false;
+  HasUnaligned = true;
+  HasFullFP16 = false;
+  HasDotProd = false;
+  HasFP16FML = false;
+  HasMTE = false;
   ArchKind = llvm::AArch64::ArchKind::ARMV8A;
 
   for (const auto &Feature : Features) {
@@ -278,11 +278,11 @@
 if (Feature == "+sve")
   FPU |= SveMode;
 if (Feature == "+crc")
-  CRC = 1;
+  HasCRC = true;
 if (Feature == "+crypto")
-  Crypto = 1;
+  HasCrypto = true;
 if (Feature == "+strict-align")
-  Unaligned = 0;
+  HasUnaligned = false;
 if (Feature == "+v8.1a")
   ArchKind = llvm::AArch64::ArchKind::ARMV8_1A;
 if (Feature == "+v8.2a")
@@ -294,13 +294,13 @@
 if (Feature == "+v8.5a")
   ArchKind = llvm::AArch64::ArchKind::ARMV8_5A;
 if (Feature == "+fullfp16")
-  HasFullFP16 = 1;
+  HasFullFP16 = true;
 if (Feature == "+dotprod")
-  HasDotProd = 1;
+  HasDotProd = true;
 if (Feature == "+fp16fml")
-  HasFP16FML = 1;
+  HasFP16FML = true;
 if (Feature == "+mte")
-  HasMTE = 1;
+  HasMTE = true;
   }
 
   setDataLayout();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64416: [AArch64] Add support for Transactional Memory Extension (TME)

[Momchil Velikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL366322: [AArch64] Add support for Transactional Memory 
Extension (TME) (authored by chill, committed by ).
Herald added a subscriber: kristina.

Changed prior to commit:
  https://reviews.llvm.org/D64416?vs=209188&id=210310#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64416

Files:
  cfe/trunk/include/clang/Basic/BuiltinsAArch64.def
  cfe/trunk/lib/Basic/Targets/AArch64.cpp
  cfe/trunk/lib/Basic/Targets/AArch64.h
  cfe/trunk/lib/Headers/arm_acle.h
  cfe/trunk/lib/Sema/SemaChecking.cpp
  cfe/trunk/test/CodeGen/aarch64-tme-tcancel-arg.cpp
  cfe/trunk/test/CodeGen/aarch64-tme.c
  cfe/trunk/test/Sema/aarch64-tme-errors.c
  cfe/trunk/test/Sema/aarch64-tme-tcancel-const-error.c
  cfe/trunk/test/Sema/aarch64-tme-tcancel-range-error.c
  llvm/trunk/include/llvm/IR/IntrinsicsAArch64.td
  llvm/trunk/include/llvm/Support/AArch64TargetParser.def
  llvm/trunk/include/llvm/Support/AArch64TargetParser.h
  llvm/trunk/lib/Target/AArch64/AArch64.td
  llvm/trunk/lib/Target/AArch64/AArch64InstrFormats.td
  llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/trunk/lib/Target/AArch64/AArch64Subtarget.h
  llvm/trunk/test/CodeGen/AArch64/tme-tcancel.ll
  llvm/trunk/test/CodeGen/AArch64/tme-tcommit.ll
  llvm/trunk/test/CodeGen/AArch64/tme-tstart.ll
  llvm/trunk/test/CodeGen/AArch64/tme-ttest.ll
  llvm/trunk/test/MC/AArch64/tme-error.s
  llvm/trunk/test/MC/AArch64/tme.s
  llvm/trunk/test/MC/Disassembler/AArch64/tme.txt
  llvm/trunk/unittests/Support/TargetParserTest.cpp

Index: llvm/trunk/include/llvm/Support/AArch64TargetParser.def
===
--- llvm/trunk/include/llvm/Support/AArch64TargetParser.def
+++ llvm/trunk/include/llvm/Support/AArch64TargetParser.def
@@ -79,6 +79,7 @@
 AARCH64_ARCH_EXT_NAME("ssbs",  AArch64::AEK_SSBS, "+ssbs",  "-ssbs")
 AARCH64_ARCH_EXT_NAME("sb",AArch64::AEK_SB,   "+sb","-sb")
 AARCH64_ARCH_EXT_NAME("predres",   AArch64::AEK_PREDRES,  "+predres", "-predres")
+AARCH64_ARCH_EXT_NAME("tme",   AArch64::AEK_TME,  "+tme",   "-tme")
 #undef AARCH64_ARCH_EXT_NAME
 
 #ifndef AARCH64_CPU_NAME
Index: llvm/trunk/include/llvm/Support/AArch64TargetParser.h
===
--- llvm/trunk/include/llvm/Support/AArch64TargetParser.h
+++ llvm/trunk/include/llvm/Support/AArch64TargetParser.h
@@ -54,6 +54,7 @@
   AEK_SVE2SM4 = 1 << 25,
   AEK_SVE2SHA3 =1 << 26,
   AEK_BITPERM = 1 << 27,
+  AEK_TME = 1 << 28,
 };
 
 enum class ArchKind {
Index: llvm/trunk/include/llvm/IR/IntrinsicsAArch64.td
===
--- llvm/trunk/include/llvm/IR/IntrinsicsAArch64.td
+++ llvm/trunk/include/llvm/IR/IntrinsicsAArch64.td
@@ -703,3 +703,20 @@
 def int_aarch64_subp :  Intrinsic<[llvm_i64_ty], [llvm_ptr_ty, llvm_ptr_ty],
 [IntrNoMem]>;
 }
+
+// Transactional Memory Extension (TME) Intrinsics
+let TargetPrefix = "aarch64" in {
+def int_aarch64_tstart  : GCCBuiltin<"__builtin_arm_tstart">,
+ Intrinsic<[llvm_i64_ty]>;
+
+def int_aarch64_tcommit : GCCBuiltin<"__builtin_arm_tcommit">, Intrinsic<[]>;
+
+def int_aarch64_tcancel : GCCBuiltin<"__builtin_arm_tcancel">,
+  Intrinsic<[], [llvm_i64_ty],
+[ImmArg<0>, IntrNoMem, IntrHasSideEffects,
+ IntrNoReturn]>;
+
+def int_aarch64_ttest   : GCCBuiltin<"__builtin_arm_ttest">,
+  Intrinsic<[llvm_i64_ty], [],
+[IntrNoMem, IntrHasSideEffects]>;
+}
Index: llvm/trunk/test/MC/AArch64/tme.s
===
--- llvm/trunk/test/MC/AArch64/tme.s
+++ llvm/trunk/test/MC/AArch64/tme.s
@@ -0,0 +1,24 @@
+// Tests for transaction memory extension instructions
+//
+// RUN: llvm-mc -triple aarch64 -show-encoding -mattr=+tme   < %s  | FileCheck %s
+// RUN: not llvm-mc -triple aarch64 -show-encoding -mattr=-tme   < %s 2>&1 | FileCheck %s --check-prefix=NOTME
+
+tstart x3
+ttest  x4
+tcommit
+tcancel #0x1234
+
+// CHECK: tstart x3 // encoding: [0x63,0x30,0x23,0xd5]
+// CHECK: ttest x4  // encoding: [0x64,0x31,0x23,0xd5]
+// CHECK: tcommit   // encoding: [0x7f,0x30,0x03,0xd5]
+// CHECK: tcancel #0x1234   // encoding: [0x80,0x46,0x62,0xd4]
+
+
+// NOTME: instruction requires: tme
+// NOTME-NEXT: tstart x3
+// NOTME: instruction requires: tme
+// NOTME-NEXT: ttest  x4
+// NOTME: instruction requires: tme
+// NOTME-NEXT: tcommit
+// NOTME: instruction requires: tme
+// NOTME-NEXT: tcancel #0x1234
Index: llvm/trunk/test/MC/AArch64/tme-error.s
===
--- llvm/trunk/test/MC/AArch64/tme-error.s
+++ llvm/trunk/test/MC/AArch64/tm

[PATCH] D64416: [AArch64] Add support for Transactional Memory Extension (TME)

[Momchil Velikov via Phabricator via cfe-commits]

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

I reverted the patch, have to rework `tcancel`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64416



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

[PATCH] D64416: [AArch64] Add support for Transactional Memory Extension (TME)

[Momchil Velikov via Phabricator via cfe-commits]

chill updated this revision to Diff 210768.
chill added a comment.
This revision is now accepted and ready to land.

Changed `tcancel` implementation.


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

https://reviews.llvm.org/D64416

Files:
  clang/include/clang/Basic/BuiltinsAArch64.def
  clang/lib/Basic/Targets/AArch64.cpp
  clang/lib/Basic/Targets/AArch64.h
  clang/lib/Headers/arm_acle.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/aarch64-tme-tcancel-arg.cpp
  clang/test/CodeGen/aarch64-tme.c
  clang/test/Sema/aarch64-tme-errors.c
  clang/test/Sema/aarch64-tme-tcancel-const-error.c
  clang/test/Sema/aarch64-tme-tcancel-range-error.c
  llvm/include/llvm/IR/IntrinsicsAArch64.td
  llvm/include/llvm/Support/AArch64TargetParser.def
  llvm/include/llvm/Support/AArch64TargetParser.h
  llvm/lib/Target/AArch64/AArch64.td
  llvm/lib/Target/AArch64/AArch64InstrFormats.td
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/lib/Target/AArch64/AArch64Subtarget.h
  llvm/test/CodeGen/AArch64/tme-tcancel.ll
  llvm/test/CodeGen/AArch64/tme-tcommit.ll
  llvm/test/CodeGen/AArch64/tme-tstart.ll
  llvm/test/CodeGen/AArch64/tme-ttest.ll
  llvm/test/MC/AArch64/tme-error.s
  llvm/test/MC/AArch64/tme.s
  llvm/test/MC/Disassembler/AArch64/tme.txt
  llvm/unittests/Support/TargetParserTest.cpp

Index: llvm/unittests/Support/TargetParserTest.cpp
===
--- llvm/unittests/Support/TargetParserTest.cpp
+++ llvm/unittests/Support/TargetParserTest.cpp
@@ -1119,6 +1119,7 @@
   {"rcpc", "norcpc", "+rcpc", "-rcpc" },
   {"rng", "norng", "+rand", "-rand"},
   {"memtag", "nomemtag", "+mte", "-mte"},
+  {"tme", "notme", "+tme", "-tme"},
   {"ssbs", "nossbs", "+ssbs", "-ssbs"},
   {"sb", "nosb", "+sb", "-sb"},
   {"predres", "nopredres", "+predres", "-predres"}
Index: llvm/test/MC/Disassembler/AArch64/tme.txt
===
--- /dev/null
+++ llvm/test/MC/Disassembler/AArch64/tme.txt
@@ -0,0 +1,19 @@
+# Tests for transaction memory extension instructions
+# RUN: llvm-mc -triple=aarch64 -mattr=+tme   -disassemble < %s  | FileCheck %s
+# RUN: not llvm-mc -triple=aarch64 -mattr=-tme   -disassemble < %s 2>&1 | FileCheck %s --check-prefix=NOTME
+
+[0x63,0x30,0x23,0xd5]
+[0x64,0x31,0x23,0xd5]
+[0x7f,0x30,0x03,0xd5]
+[0x80,0x46,0x62,0xd4]
+
+# CHECK: tstart x3
+# CHECK: ttest  x4
+# CHECK: tcommit
+# CHECK: tcancel #0x1234
+
+# NOTEME: mrs
+# NOTEME-NEXT: mrs
+# NOTEME-NEXT: msr
+# NOTME:  warning: invalid instruction encoding
+# NOTME-NEXT: [0x80,0x46,0x62,0xd4]
Index: llvm/test/MC/AArch64/tme.s
===
--- /dev/null
+++ llvm/test/MC/AArch64/tme.s
@@ -0,0 +1,24 @@
+// Tests for transaction memory extension instructions
+//
+// RUN: llvm-mc -triple aarch64 -show-encoding -mattr=+tme   < %s  | FileCheck %s
+// RUN: not llvm-mc -triple aarch64 -show-encoding -mattr=-tme   < %s 2>&1 | FileCheck %s --check-prefix=NOTME
+
+tstart x3
+ttest  x4
+tcommit
+tcancel #0x1234
+
+// CHECK: tstart x3 // encoding: [0x63,0x30,0x23,0xd5]
+// CHECK: ttest x4  // encoding: [0x64,0x31,0x23,0xd5]
+// CHECK: tcommit   // encoding: [0x7f,0x30,0x03,0xd5]
+// CHECK: tcancel #0x1234   // encoding: [0x80,0x46,0x62,0xd4]
+
+
+// NOTME: instruction requires: tme
+// NOTME-NEXT: tstart x3
+// NOTME: instruction requires: tme
+// NOTME-NEXT: ttest  x4
+// NOTME: instruction requires: tme
+// NOTME-NEXT: tcommit
+// NOTME: instruction requires: tme
+// NOTME-NEXT: tcancel #0x1234
Index: llvm/test/MC/AArch64/tme-error.s
===
--- /dev/null
+++ llvm/test/MC/AArch64/tme-error.s
@@ -0,0 +1,47 @@
+// Tests for transactional memory extension instructions
+// RUN: not llvm-mc -triple aarch64 -show-encoding -mattr=+tme < %s 2>&1   | FileCheck %s
+
+tstart
+// CHECK: error: too few operands for instruction
+// CHECK-NEXT: tstart
+tstart  x4, x5
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: tstart x4, x5
+tstart  x4, #1
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: tstart x4, #1
+tstart  sp
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: tstart sp
+
+ttest
+// CHECK: error: too few operands for instruction
+// CHECK-NEXT: ttest
+ttest  x4, x5
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: ttest x4, x5
+ttest  x4, #1
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: ttest x4, #1
+ttest  sp
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: ttest sp
+
+tcommit  x4
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: tcommit x4
+tcommit  sp
+// CHECK: error: invalid operand for ins

[PATCH] D64416: [AArch64] Add support for Transactional Memory Extension (TME)

[Momchil Velikov via Phabricator via cfe-commits]

chill updated this revision to Diff 211140.
This revision is now accepted and ready to land.

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

https://reviews.llvm.org/D64416

Files:
  clang/include/clang/Basic/BuiltinsAArch64.def
  clang/lib/Basic/Targets/AArch64.cpp
  clang/lib/Basic/Targets/AArch64.h
  clang/lib/Headers/arm_acle.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/aarch64-tme.cpp
  clang/test/Sema/aarch64-tme-errors.c
  clang/test/Sema/aarch64-tme-tcancel-errors.c
  llvm/include/llvm/IR/IntrinsicsAArch64.td
  llvm/include/llvm/Support/AArch64TargetParser.def
  llvm/include/llvm/Support/AArch64TargetParser.h
  llvm/lib/Target/AArch64/AArch64.td
  llvm/lib/Target/AArch64/AArch64InstrFormats.td
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/lib/Target/AArch64/AArch64Subtarget.h
  llvm/test/CodeGen/AArch64/tme.ll
  llvm/test/MC/AArch64/tme-error.s
  llvm/test/MC/AArch64/tme.s
  llvm/test/MC/Disassembler/AArch64/tme.txt
  llvm/unittests/Support/TargetParserTest.cpp

Index: llvm/unittests/Support/TargetParserTest.cpp
===
--- llvm/unittests/Support/TargetParserTest.cpp
+++ llvm/unittests/Support/TargetParserTest.cpp
@@ -1119,6 +1119,7 @@
   {"rcpc", "norcpc", "+rcpc", "-rcpc" },
   {"rng", "norng", "+rand", "-rand"},
   {"memtag", "nomemtag", "+mte", "-mte"},
+  {"tme", "notme", "+tme", "-tme"},
   {"ssbs", "nossbs", "+ssbs", "-ssbs"},
   {"sb", "nosb", "+sb", "-sb"},
   {"predres", "nopredres", "+predres", "-predres"}
Index: llvm/test/MC/Disassembler/AArch64/tme.txt
===
--- /dev/null
+++ llvm/test/MC/Disassembler/AArch64/tme.txt
@@ -0,0 +1,19 @@
+# Tests for transaction memory extension instructions
+# RUN: llvm-mc -triple=aarch64 -mattr=+tme   -disassemble < %s  | FileCheck %s
+# RUN: not llvm-mc -triple=aarch64 -mattr=-tme   -disassemble < %s 2>&1 | FileCheck %s --check-prefix=NOTME
+
+[0x63,0x30,0x23,0xd5]
+[0x64,0x31,0x23,0xd5]
+[0x7f,0x30,0x03,0xd5]
+[0x80,0x46,0x62,0xd4]
+
+# CHECK: tstart x3
+# CHECK: ttest  x4
+# CHECK: tcommit
+# CHECK: tcancel #0x1234
+
+# NOTEME: mrs
+# NOTEME-NEXT: mrs
+# NOTEME-NEXT: msr
+# NOTME:  warning: invalid instruction encoding
+# NOTME-NEXT: [0x80,0x46,0x62,0xd4]
Index: llvm/test/MC/AArch64/tme.s
===
--- /dev/null
+++ llvm/test/MC/AArch64/tme.s
@@ -0,0 +1,24 @@
+// Tests for transaction memory extension instructions
+//
+// RUN: llvm-mc -triple aarch64 -show-encoding -mattr=+tme   < %s  | FileCheck %s
+// RUN: not llvm-mc -triple aarch64 -show-encoding -mattr=-tme   < %s 2>&1 | FileCheck %s --check-prefix=NOTME
+
+tstart x3
+ttest  x4
+tcommit
+tcancel #0x1234
+
+// CHECK: tstart x3 // encoding: [0x63,0x30,0x23,0xd5]
+// CHECK: ttest x4  // encoding: [0x64,0x31,0x23,0xd5]
+// CHECK: tcommit   // encoding: [0x7f,0x30,0x03,0xd5]
+// CHECK: tcancel #0x1234   // encoding: [0x80,0x46,0x62,0xd4]
+
+
+// NOTME: instruction requires: tme
+// NOTME-NEXT: tstart x3
+// NOTME: instruction requires: tme
+// NOTME-NEXT: ttest  x4
+// NOTME: instruction requires: tme
+// NOTME-NEXT: tcommit
+// NOTME: instruction requires: tme
+// NOTME-NEXT: tcancel #0x1234
Index: llvm/test/MC/AArch64/tme-error.s
===
--- /dev/null
+++ llvm/test/MC/AArch64/tme-error.s
@@ -0,0 +1,47 @@
+// Tests for transactional memory extension instructions
+// RUN: not llvm-mc -triple aarch64 -show-encoding -mattr=+tme < %s 2>&1   | FileCheck %s
+
+tstart
+// CHECK: error: too few operands for instruction
+// CHECK-NEXT: tstart
+tstart  x4, x5
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: tstart x4, x5
+tstart  x4, #1
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: tstart x4, #1
+tstart  sp
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: tstart sp
+
+ttest
+// CHECK: error: too few operands for instruction
+// CHECK-NEXT: ttest
+ttest  x4, x5
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: ttest x4, x5
+ttest  x4, #1
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: ttest x4, #1
+ttest  sp
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: ttest sp
+
+tcommit  x4
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: tcommit x4
+tcommit  sp
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: tcommit sp
+
+
+tcancel
+// CHECK: error: too few operands for instruction
+// CHECK-NEXT tcancel
+tcancel x0
+// CHECK: error: immediate must be an integer in range [0, 65535]
+// CHECK-NEXT tcancel
+tcancel #65536
+// CHECK: error: immediate must be an integer in range 

[PATCH] D64416: [AArch64] Add support for Transactional Memory Extension (TME)

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

If there are no objections, I'll go ahead with committing that Soon(tm) on the 
basis of previous acceptance.


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

https://reviews.llvm.org/D64416



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

[PATCH] D64416: [AArch64] Add support for Transactional Memory Extension (TME)

[Momchil Velikov via Phabricator via cfe-commits]

chill updated this revision to Diff 212332.
chill added a comment.

Rebase on top of master.


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

https://reviews.llvm.org/D64416

Files:
  clang/include/clang/Basic/BuiltinsAArch64.def
  clang/lib/Basic/Targets/AArch64.cpp
  clang/lib/Basic/Targets/AArch64.h
  clang/lib/Headers/arm_acle.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/aarch64-tme.cpp
  clang/test/Sema/aarch64-tme-errors.c
  clang/test/Sema/aarch64-tme-tcancel-errors.c
  llvm/include/llvm/IR/IntrinsicsAArch64.td
  llvm/include/llvm/Support/AArch64TargetParser.def
  llvm/include/llvm/Support/AArch64TargetParser.h
  llvm/lib/Target/AArch64/AArch64.td
  llvm/lib/Target/AArch64/AArch64InstrFormats.td
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/lib/Target/AArch64/AArch64Subtarget.h
  llvm/test/CodeGen/AArch64/tme.ll
  llvm/test/MC/AArch64/tme-error.s
  llvm/test/MC/AArch64/tme.s
  llvm/test/MC/Disassembler/AArch64/tme.txt
  llvm/unittests/Support/TargetParserTest.cpp

Index: llvm/unittests/Support/TargetParserTest.cpp
===
--- llvm/unittests/Support/TargetParserTest.cpp
+++ llvm/unittests/Support/TargetParserTest.cpp
@@ -1153,6 +1153,7 @@
   {"rcpc", "norcpc", "+rcpc", "-rcpc" },
   {"rng", "norng", "+rand", "-rand"},
   {"memtag", "nomemtag", "+mte", "-mte"},
+  {"tme", "notme", "+tme", "-tme"},
   {"ssbs", "nossbs", "+ssbs", "-ssbs"},
   {"sb", "nosb", "+sb", "-sb"},
   {"predres", "nopredres", "+predres", "-predres"}
Index: llvm/test/MC/Disassembler/AArch64/tme.txt
===
--- /dev/null
+++ llvm/test/MC/Disassembler/AArch64/tme.txt
@@ -0,0 +1,19 @@
+# Tests for transaction memory extension instructions
+# RUN: llvm-mc -triple=aarch64 -mattr=+tme   -disassemble < %s  | FileCheck %s
+# RUN: not llvm-mc -triple=aarch64 -mattr=-tme   -disassemble < %s 2>&1 | FileCheck %s --check-prefix=NOTME
+
+[0x63,0x30,0x23,0xd5]
+[0x64,0x31,0x23,0xd5]
+[0x7f,0x30,0x03,0xd5]
+[0x80,0x46,0x62,0xd4]
+
+# CHECK: tstart x3
+# CHECK: ttest  x4
+# CHECK: tcommit
+# CHECK: tcancel #0x1234
+
+# NOTEME: mrs
+# NOTEME-NEXT: mrs
+# NOTEME-NEXT: msr
+# NOTME:  warning: invalid instruction encoding
+# NOTME-NEXT: [0x80,0x46,0x62,0xd4]
Index: llvm/test/MC/AArch64/tme.s
===
--- /dev/null
+++ llvm/test/MC/AArch64/tme.s
@@ -0,0 +1,24 @@
+// Tests for transaction memory extension instructions
+//
+// RUN: llvm-mc -triple aarch64 -show-encoding -mattr=+tme   < %s  | FileCheck %s
+// RUN: not llvm-mc -triple aarch64 -show-encoding -mattr=-tme   < %s 2>&1 | FileCheck %s --check-prefix=NOTME
+
+tstart x3
+ttest  x4
+tcommit
+tcancel #0x1234
+
+// CHECK: tstart x3 // encoding: [0x63,0x30,0x23,0xd5]
+// CHECK: ttest x4  // encoding: [0x64,0x31,0x23,0xd5]
+// CHECK: tcommit   // encoding: [0x7f,0x30,0x03,0xd5]
+// CHECK: tcancel #0x1234   // encoding: [0x80,0x46,0x62,0xd4]
+
+
+// NOTME: instruction requires: tme
+// NOTME-NEXT: tstart x3
+// NOTME: instruction requires: tme
+// NOTME-NEXT: ttest  x4
+// NOTME: instruction requires: tme
+// NOTME-NEXT: tcommit
+// NOTME: instruction requires: tme
+// NOTME-NEXT: tcancel #0x1234
Index: llvm/test/MC/AArch64/tme-error.s
===
--- /dev/null
+++ llvm/test/MC/AArch64/tme-error.s
@@ -0,0 +1,47 @@
+// Tests for transactional memory extension instructions
+// RUN: not llvm-mc -triple aarch64 -show-encoding -mattr=+tme < %s 2>&1   | FileCheck %s
+
+tstart
+// CHECK: error: too few operands for instruction
+// CHECK-NEXT: tstart
+tstart  x4, x5
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: tstart x4, x5
+tstart  x4, #1
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: tstart x4, #1
+tstart  sp
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: tstart sp
+
+ttest
+// CHECK: error: too few operands for instruction
+// CHECK-NEXT: ttest
+ttest  x4, x5
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: ttest x4, x5
+ttest  x4, #1
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: ttest x4, #1
+ttest  sp
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: ttest sp
+
+tcommit  x4
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: tcommit x4
+tcommit  sp
+// CHECK: error: invalid operand for instruction
+// CHECK-NEXT: tcommit sp
+
+
+tcancel
+// CHECK: error: too few operands for instruction
+// CHECK-NEXT tcancel
+tcancel x0
+// CHECK: error: immediate must be an integer in range [0, 65535]
+// CHECK-NEXT tcancel
+tcancel #65536
+// CHECK: error: immediate must be an integer in range

[PATCH] D64416: [AArch64] Add support for Transactional Memory Extension (TME)

[Momchil Velikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL367428: [AArch64] Add support for Transactional Memory 
Extension (TME) (authored by chill, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D64416?vs=212332&id=212559#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64416

Files:
  cfe/trunk/include/clang/Basic/BuiltinsAArch64.def
  cfe/trunk/lib/Basic/Targets/AArch64.cpp
  cfe/trunk/lib/Basic/Targets/AArch64.h
  cfe/trunk/lib/Headers/arm_acle.h
  cfe/trunk/lib/Sema/SemaChecking.cpp
  cfe/trunk/test/CodeGen/aarch64-tme.cpp
  cfe/trunk/test/Sema/aarch64-tme-errors.c
  cfe/trunk/test/Sema/aarch64-tme-tcancel-errors.c
  llvm/trunk/include/llvm/IR/IntrinsicsAArch64.td
  llvm/trunk/include/llvm/Support/AArch64TargetParser.def
  llvm/trunk/include/llvm/Support/AArch64TargetParser.h
  llvm/trunk/lib/Target/AArch64/AArch64.td
  llvm/trunk/lib/Target/AArch64/AArch64InstrFormats.td
  llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/trunk/lib/Target/AArch64/AArch64Subtarget.h
  llvm/trunk/test/CodeGen/AArch64/tme.ll
  llvm/trunk/test/MC/AArch64/tme-error.s
  llvm/trunk/test/MC/AArch64/tme.s
  llvm/trunk/test/MC/Disassembler/AArch64/tme.txt
  llvm/trunk/unittests/Support/TargetParserTest.cpp

Index: llvm/trunk/include/llvm/Support/AArch64TargetParser.def
===
--- llvm/trunk/include/llvm/Support/AArch64TargetParser.def
+++ llvm/trunk/include/llvm/Support/AArch64TargetParser.def
@@ -79,6 +79,7 @@
 AARCH64_ARCH_EXT_NAME("ssbs", AArch64::AEK_SSBS,"+ssbs",  "-ssbs")
 AARCH64_ARCH_EXT_NAME("sb",   AArch64::AEK_SB,  "+sb","-sb")
 AARCH64_ARCH_EXT_NAME("predres",  AArch64::AEK_PREDRES, "+predres", "-predres")
+AARCH64_ARCH_EXT_NAME("tme",  AArch64::AEK_TME, "+tme",   "-tme")
 #undef AARCH64_ARCH_EXT_NAME
 
 #ifndef AARCH64_CPU_NAME
Index: llvm/trunk/include/llvm/Support/AArch64TargetParser.h
===
--- llvm/trunk/include/llvm/Support/AArch64TargetParser.h
+++ llvm/trunk/include/llvm/Support/AArch64TargetParser.h
@@ -54,6 +54,7 @@
   AEK_SVE2SM4 = 1 << 25,
   AEK_SVE2SHA3 =1 << 26,
   AEK_SVE2BITPERM = 1 << 27,
+  AEK_TME = 1 << 28,
 };
 
 enum class ArchKind {
Index: llvm/trunk/include/llvm/IR/IntrinsicsAArch64.td
===
--- llvm/trunk/include/llvm/IR/IntrinsicsAArch64.td
+++ llvm/trunk/include/llvm/IR/IntrinsicsAArch64.td
@@ -733,3 +733,18 @@
 def int_aarch64_stgp  : Intrinsic<[], [llvm_ptr_ty, llvm_i64_ty, llvm_i64_ty],
 [IntrWriteMem, IntrArgMemOnly, NoCapture<0>, WriteOnly<0>]>;
 }
+
+// Transactional Memory Extension (TME) Intrinsics
+let TargetPrefix = "aarch64" in {
+def int_aarch64_tstart  : GCCBuiltin<"__builtin_arm_tstart">,
+ Intrinsic<[llvm_i64_ty]>;
+
+def int_aarch64_tcommit : GCCBuiltin<"__builtin_arm_tcommit">, Intrinsic<[]>;
+
+def int_aarch64_tcancel : GCCBuiltin<"__builtin_arm_tcancel">,
+  Intrinsic<[], [llvm_i64_ty], [ImmArg<0>]>;
+
+def int_aarch64_ttest   : GCCBuiltin<"__builtin_arm_ttest">,
+  Intrinsic<[llvm_i64_ty], [],
+[IntrNoMem, IntrHasSideEffects]>;
+}
Index: llvm/trunk/test/CodeGen/AArch64/tme.ll
===
--- llvm/trunk/test/CodeGen/AArch64/tme.ll
+++ llvm/trunk/test/CodeGen/AArch64/tme.ll
@@ -0,0 +1,44 @@
+; RUN: llc %s -verify-machineinstrs -o - | FileCheck %s
+
+target triple = "aarch64-unknown-unknown-eabi"
+
+define i64 @test_tstart() #0 {
+  %r = tail call i64 @llvm.aarch64.tstart()
+  ret i64 %r
+}
+declare i64 @llvm.aarch64.tstart() #1
+; CHECK-LABEL: test_tstart
+; CHECK: tstart x
+
+define i64 @test_ttest() #0 {
+  %r = tail call i64 @llvm.aarch64.ttest()
+  ret i64 %r
+}
+declare i64 @llvm.aarch64.ttest() #1
+; CHECK-LABEL: test_ttest
+; CHECK: ttest x
+
+define void @test_tcommit() #0 {
+  tail call void @llvm.aarch64.tcommit()
+  ret void
+}
+declare void @llvm.aarch64.tcommit() #1
+; CHECK-LABEL: test_tcommit
+; CHECK: tcommit
+
+define void @test_tcancel() #0 {
+  tail call void @llvm.aarch64.tcancel(i64 0) #1
+  tail call void @llvm.aarch64.tcancel(i64 1) #1
+  tail call void @llvm.aarch64.tcancel(i64 65534) #1
+  tail call void @llvm.aarch64.tcancel(i64 65535) #1
+  ret void
+}
+declare void @llvm.aarch64.tcancel(i64 immarg) #1
+; CHECK-LABEL: test_tcancel
+; CHECK: tcancel #0
+; CHECK: tcancel #0x1
+; CHECK: tcancel #0xfffe
+; CHECK: tcancel #0x
+
+attributes #0 = { "target-features"="+tme" }
+attributes #1 = { nounwind }
Index: llvm/trunk/test/MC/AArch64/tme-error.s
===
--- llvm/trunk/test/MC/AArch64/tme-error.s
+++ llvm/trunk/test/MC/AArch64/tme

[PATCH] D64410: [WIP] Intrinsics side effects

[Momchil Velikov via Phabricator via cfe-commits]

chill created this revision.
Herald added subscribers: llvm-commits, cfe-commits, hiraditya, kristof.beyls, 
javed.absar.
Herald added projects: clang, LLVM.

Change-Id: I49f5612ce6899004fa84b9a83c34dcd2d9af8224

Consistent types and naming for AArch64 target features (NFC)

Change-Id: I6bac2672de675a6e1c9c8867ed81bef879adc417

[AArch64] Add support for Transactional Memory Extension (TME)

TME is a future architecture technology, documented in

https://developer.arm.com/architectures/cpu-architecture/a-profile/exploration-tools
https://developer.arm.com/docs/ddi0601/a

This patch adds support for the TME instructions TSTART, TTEST, TCOMMIT, and
TCANCEL and the target feature/arch extension "tme".

It alaso implements TME builtin functions, defined in ACLE vX.Y spec.

Change-Id: If77e296a68d1e30d584808677b1ce96be1ac2420


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64410

Files:
  clang/include/clang/Basic/BuiltinsAArch64.def
  clang/lib/Basic/Targets/AArch64.cpp
  clang/lib/Basic/Targets/AArch64.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Headers/arm_acle.h
  clang/test/CodeGen/aarch64-tme-errors.c
  clang/test/CodeGen/aarch64-tme-tcancel-arg.cpp
  clang/test/CodeGen/aarch64-tme-tcancel-const-error.c
  clang/test/CodeGen/aarch64-tme-tcancel-range-error.c
  clang/test/CodeGen/aarch64-tme.c
  llvm/include/llvm/IR/IntrinsicsAArch64.td
  llvm/include/llvm/Support/AArch64TargetParser.def
  llvm/include/llvm/Support/AArch64TargetParser.h
  llvm/lib/Target/AArch64/AArch64.td
  llvm/lib/Target/AArch64/AArch64InstrFormats.td
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/lib/Target/AArch64/AArch64Subtarget.h
  llvm/test/CodeGen/AArch64/tme.ll
  llvm/test/MC/AArch64/tme-error.s
  llvm/test/MC/AArch64/tme.s
  llvm/test/MC/Disassembler/AArch64/tme.txt
  llvm/unittests/Support/TargetParserTest.cpp
  llvm/utils/TableGen/CodeGenDAGPatterns.cpp
  llvm/utils/TableGen/IntrinsicEmitter.cpp

Index: llvm/utils/TableGen/IntrinsicEmitter.cpp
===
--- llvm/utils/TableGen/IntrinsicEmitter.cpp
+++ llvm/utils/TableGen/IntrinsicEmitter.cpp
@@ -685,7 +685,7 @@
 }
 
 if (!intrinsic.canThrow ||
-intrinsic.ModRef != CodeGenIntrinsic::ReadWriteMem ||
+(intrinsic.ModRef != CodeGenIntrinsic::ReadWriteMem && !intrinsic.hasSideEffects) ||
 intrinsic.isNoReturn || intrinsic.isCold || intrinsic.isNoDuplicate ||
 intrinsic.isConvergent || intrinsic.isSpeculatable) {
   OS << "  const Attribute::AttrKind Atts[] = {";
@@ -727,6 +727,8 @@
 
   switch (intrinsic.ModRef) {
   case CodeGenIntrinsic::NoMem:
+if (intrinsic.hasSideEffects)
+  break;
 if (addComma)
   OS << ",";
 OS << "Attribute::ReadNone";
Index: llvm/utils/TableGen/CodeGenDAGPatterns.cpp
===
--- llvm/utils/TableGen/CodeGenDAGPatterns.cpp
+++ llvm/utils/TableGen/CodeGenDAGPatterns.cpp
@@ -2779,7 +2779,7 @@
 // chain.
 if (Int.IS.RetVTs.empty())
   Operator = getDAGPatterns().get_intrinsic_void_sdnode();
-else if (Int.ModRef != CodeGenIntrinsic::NoMem)
+else if (Int.ModRef != CodeGenIntrinsic::NoMem || Int.hasSideEffects)
   // Has side-effects, requires chain.
   Operator = getDAGPatterns().get_intrinsic_w_chain_sdnode();
 else // Otherwise, no chain.
Index: llvm/unittests/Support/TargetParserTest.cpp
===
--- llvm/unittests/Support/TargetParserTest.cpp
+++ llvm/unittests/Support/TargetParserTest.cpp
@@ -1118,6 +1118,7 @@
   {"rcpc", "norcpc", "+rcpc", "-rcpc" },
   {"rng", "norng", "+rand", "-rand"},
   {"memtag", "nomemtag", "+mte", "-mte"},
+  {"tme", "notme", "+tme", "-tme"},
   {"ssbs", "nossbs", "+ssbs", "-ssbs"},
   {"sb", "nosb", "+sb", "-sb"},
   {"predres", "nopredres", "+predres", "-predres"}
Index: llvm/test/MC/Disassembler/AArch64/tme.txt
===
--- /dev/null
+++ llvm/test/MC/Disassembler/AArch64/tme.txt
@@ -0,0 +1,19 @@
+# Tests for transaction memory extension instructions
+# RUN: llvm-mc -triple=aarch64 -mattr=+tme   -disassemble < %s  | FileCheck %s
+# RUN: not llvm-mc -triple=aarch64 -mattr=-tme   -disassemble < %s 2>&1 | FileCheck %s --check-prefix=NOTME
+
+[0x63,0x30,0x23,0xd5]
+[0x64,0x31,0x23,0xd5]
+[0x7f,0x30,0x03,0xd5]
+[0x80,0x46,0x62,0xd4]
+
+# CHECK: tstart x3
+# CHECK: ttest  x4
+# CHECK: tcommit
+# CHECK: tcancel #0x1234
+
+# NOTEME: mrs
+# NOTEME-NEXT: mrs
+ NOTEME-NEXT: msr
+# NOTME:  warning: invalid instruction encoding
+# NOTME-NEXT: [0x80,0x46,0x62,0xd4]
Index: llvm/test/MC/AArch64/tme.s
==

[PATCH] D64410: [WIP] Intrinsics side effects

[Momchil Velikov via Phabricator via cfe-commits]

chill abandoned this revision.
chill added a comment.

I fail at arcanist.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64410



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

[PATCH] D67542: Fix depfile name construction

[Momchil Velikov via Phabricator via cfe-commits]

chill added a reviewer: chill.
chill added inline comments.



Comment at: clang/test/Driver/metadata-with-dots.c:2
+// RUN: mkdir -p out.dir
+// RUN: cat %s > out.dir/test.c
+// RUN: %clang -E -MMD %s -o out.dir/test

Is this going to ruin on Windows ?




Comment at: clang/test/Driver/metadata-with-dots.c:4-6
+// RUN: test ! -f %out.d
+// RUN: test -f out.dir/test.d
+// RUN: rm -rf out.dir/test.d out.dir/ out.d

And these ?


Repository:
  rC Clang

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

https://reviews.llvm.org/D67542



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

[PATCH] D67542: Fix depfile name construction

[Momchil Velikov via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D67542



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

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill created this revision.
chill added reviewers: john.brawn, olista01, eli.friedman, rengolin.
Herald added a reviewer: javed.absar.
Herald added subscribers: chrib, kristof.beyls.

The "Procedure Call Procedure Call Standard for the ARM®  Architecture" 
(https://static.docs.arm.com/ihi0042/f/IHI0042F_aapcs.pdf), specifies that 
composite types are passed  according to their natural alignment:

> 5.5 Parameter Passing
>  ...
>  B.5 If the argument is an alignment adjusted type its value is passed as a 
> copy of the actual value. The
>  copy will have an alignment defined as follows.
> 
> - For a Fundamental Data Type, the alignment is the natural alignment of that 
> type, after any promotions
> - For a Composite Type, the alignment of the copy will have 4-byte alignment 
> if its natural alignment is <= 4 and 8-byte alignment if its natural 
> alignment is >= 8

The "natural alignment" is defined as the maximum of the alignment of the 
top-level components:

> 4.3 Composite Types
>  ...
> 
> - The natural alignment of a composite type is the maximum of each of the 
> member alignments of the 'top-level' members of the composite type i.e. 
> before any alignment adjustment of the entire composite is applied

`clang`,  however, uses the actual alignment of the composite, instead of the 
natural alignment.

This patch fixes passing of composite types to use the natural alignment.  With 
this patch `clang` conforms to AAPCS and is compatible with GCC.


Repository:
  rC Clang

https://reviews.llvm.org/D46013

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/RecordLayout.h
  lib/AST/ASTContext.cpp
  lib/AST/RecordLayout.cpp
  lib/AST/RecordLayoutBuilder.cpp
  lib/CodeGen/TargetInfo.cpp
  test/CodeGen/aapcs-align.cc
  test/CodeGen/arm-arguments.c

Index: test/CodeGen/arm-arguments.c
===
--- test/CodeGen/arm-arguments.c
+++ test/CodeGen/arm-arguments.c
@@ -211,10 +211,13 @@
 // APCS-GNU: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align {{[0-9]+}} %[[b]], i8* align {{[0-9]+}} %[[c]]
 // APCS-GNU: %[[d:.*]] = bitcast %struct.s35* %[[a]] to <4 x float>*
 // APCS-GNU: load <4 x float>, <4 x float>* %[[d]], align 16
-// AAPCS-LABEL: define arm_aapcscc <4 x float> @f35(i32 %i, %struct.s35* byval align 8, %struct.s35* byval align 8)
-// AAPCS: %[[a:.*]] = alloca %struct.s35, align 16
-// AAPCS: %[[b:.*]] = bitcast %struct.s35* %[[a]] to i8*
-// AAPCS: %[[c:.*]] = bitcast %struct.s35* %0 to i8*
-// AAPCS: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 16 %[[b]], i8* align 8 %[[c]]
-// AAPCS: %[[d:.*]] = bitcast %struct.s35* %[[a]] to <4 x float>*
-// AAPCS: load <4 x float>, <4 x float>* %[[d]], align 16
+
+// AAPCS-LABEL: define arm_aapcscc <4 x float> @f35(i32 %i, %struct.s35* byval align 4 %s1, %struct.s35* byval align 4 %s2)
+// AAPCS: %[[a_addr:.*]] = alloca <4 x float>, align 16
+// AAPCS: %[[b_addr:.*]] = alloca <4 x float>, align 16
+// AAPCS: %[[p1:.*]] = bitcast %struct.s35* %s1 to <4 x float>*
+// AAPCS: %[[a:.*]] = load <4 x float>, <4 x float>* %[[p1]], align 4
+// AAPCS: %[[p2:.*]] = bitcast %struct.s35* %s2 to <4 x float>*
+// AAPCS: %[[b:.*]] = load <4 x float>, <4 x float>* %[[p2]], align 4
+// AAPCS: store <4 x float> %[[a]], <4 x float>* %[[a_addr]], align 16
+// AAPCS: store <4 x float> %[[b]], <4 x float>* %[[b_addr]], align 16
Index: test/CodeGen/aapcs-align.cc
===
--- /dev/null
+++ test/CodeGen/aapcs-align.cc
@@ -0,0 +1,102 @@
+// REQUIRES: arm-registered-target
+// RUN: %clang_cc1 -triple arm-none-none-eabi \
+// RUN:   -O2 \
+// RUN:   -target-cpu cortex-a8 \
+// RUN:   -emit-llvm -o - %s | FileCheck %s
+
+extern "C" {
+
+struct S {
+  int x, y;
+};
+
+// Base case, nothing interesting.
+void f0(int, S);
+void f0m(int, int, int, int, int, S);
+void g0() {
+  S s = {6, 7};
+  f0(1, s);
+  f0m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g0
+// CHECK: call void @f0{{.*}}[2 x i32] [i32 6, i32 7]
+// CHECK: call void @f0m{{.*}}[2 x i32] [i32 6, i32 7]
+// CHECK: declare void @f0(i32, [2 x i32])
+// CHECK: declare void @f0m(i32, i32, i32, i32, i32, [2 x i32])
+
+// Aligned struct, passed according to its natural alignment.
+struct __attribute__((aligned(8))) S8 {
+  int x, y;
+} s8;
+
+void f1(int, S8);
+void f1m(int, int, int, int, int, S8 s);
+void g1() {
+  S8 s = {6, 7};
+  f1(1, s);
+  f1m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g1
+// CHECK: call void @f1{{.*}}[2 x i32] [i32 6, i32 7]
+// CHECK: call void @f1m{{.*}}[2 x i32] [i32 6, i32 7]
+// CHECK: declare void @f1(i32, [2 x i32])
+// CHECK: declare void @f1m(i32, i32, i32, i32, i32, [2 x i32])
+
+// Aligned struct, passed according to its natural alignment.
+struct alignas(16) S16 {
+  int x, y;
+};
+
+extern "C" void f2(int, S16);
+extern "C" void f2m(int, int, int, int, int, S16);
+
+void g2() {
+  S16 s = {6, 7};
+  f2(1, s);
+  f2m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g2
+// CHECK: c

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Ping?


Repository:
  rC Clang

https://reviews.llvm.org/D46013



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

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In https://reviews.llvm.org/D46013#1084440, @efriedma wrote:

> I'd like to see some tests for __attribute((packed)).


Thanks, indeed it does not work correctly on packed structures. Back to the 
drawing board ...


Repository:
  rC Clang

https://reviews.llvm.org/D46013



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

[PATCH] D46439: Fix incorrect packed aligned structure layout

[Momchil Velikov via Phabricator via cfe-commits]

chill created this revision.
chill added reviewers: rsmith, aaron.ballman.

Executing the following program

  #include 
  #include 
  
  struct S {
char x;
int y;
  } __attribute__((packed, aligned(8)));
  
  struct alignas(8) T {
char x;
int y;
  } __attribute__((packed));
  
  int main() {
assert(offsetof(S, x) == 0);
assert(offsetof(S, y) == 1);
  
assert(offsetof(T, x) == 0);
assert(offsetof(T, y) == 1);
  }

fails with assertion

  a.out: a.cc:19: int main(): Assertion `offsetof(T, y) == 1' failed.

The layout if `T` is incorrect, because it's computed and effectively cached 
when checking for `alignas` under-aligning the structure, however, this happens 
before `__attribute__((packed))` is processed.

This patch moves the processing of attributes before the `alignas` 
under-alignment check.


Repository:
  rC Clang

https://reviews.llvm.org/D46439

Files:
  lib/Sema/SemaDecl.cpp


Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -15573,6 +15573,10 @@
 if (!Completed)
   Record->completeDefinition();
 
+// Handle attributes before checking for alignas underalignment.
+if (Attr)
+  ProcessDeclAttributeList(S, Record, Attr);
+
 // We may have deferred checking for a deleted destructor. Check now.
 if (CXXRecordDecl *CXXRecord = dyn_cast(Record)) {
   auto *Dtor = CXXRecord->getDestructor();
@@ -15703,9 +15707,6 @@
   CDecl->setIvarRBraceLoc(RBrac);
 }
   }
-
-  if (Attr)
-ProcessDeclAttributeList(S, Record, Attr);
 }
 
 /// \brief Determine whether the given integral value is representable within


Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -15573,6 +15573,10 @@
 if (!Completed)
   Record->completeDefinition();
 
+// Handle attributes before checking for alignas underalignment.
+if (Attr)
+  ProcessDeclAttributeList(S, Record, Attr);
+
 // We may have deferred checking for a deleted destructor. Check now.
 if (CXXRecordDecl *CXXRecord = dyn_cast(Record)) {
   auto *Dtor = CXXRecord->getDestructor();
@@ -15703,9 +15707,6 @@
   CDecl->setIvarRBraceLoc(RBrac);
 }
   }
-
-  if (Attr)
-ProcessDeclAttributeList(S, Record, Attr);
 }
 
 /// \brief Determine whether the given integral value is representable within
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill updated this revision to Diff 152679.
chill added a comment.

Update: use "unadjusted alignment" instead of "natural alignment", rename 
things accordingly.


https://reviews.llvm.org/D46013

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/RecordLayout.h
  lib/AST/ASTContext.cpp
  lib/AST/RecordLayout.cpp
  lib/AST/RecordLayoutBuilder.cpp
  lib/CodeGen/TargetInfo.cpp
  test/CodeGen/aapcs-align.cc
  test/CodeGen/aapcs64-align.cc
  test/CodeGen/arm-arguments.c

Index: test/CodeGen/arm-arguments.c
===
--- test/CodeGen/arm-arguments.c
+++ test/CodeGen/arm-arguments.c
@@ -211,10 +211,13 @@
 // APCS-GNU: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align {{[0-9]+}} %[[b]], i8* align {{[0-9]+}} %[[c]]
 // APCS-GNU: %[[d:.*]] = bitcast %struct.s35* %[[a]] to <4 x float>*
 // APCS-GNU: load <4 x float>, <4 x float>* %[[d]], align 16
-// AAPCS-LABEL: define arm_aapcscc <4 x float> @f35(i32 %i, %struct.s35* byval align 8, %struct.s35* byval align 8)
-// AAPCS: %[[a:.*]] = alloca %struct.s35, align 16
-// AAPCS: %[[b:.*]] = bitcast %struct.s35* %[[a]] to i8*
-// AAPCS: %[[c:.*]] = bitcast %struct.s35* %0 to i8*
-// AAPCS: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 16 %[[b]], i8* align 8 %[[c]]
-// AAPCS: %[[d:.*]] = bitcast %struct.s35* %[[a]] to <4 x float>*
-// AAPCS: load <4 x float>, <4 x float>* %[[d]], align 16
+
+// AAPCS-LABEL: define arm_aapcscc <4 x float> @f35(i32 %i, %struct.s35* byval align 4 %s1, %struct.s35* byval align 4 %s2)
+// AAPCS: %[[a_addr:.*]] = alloca <4 x float>, align 16
+// AAPCS: %[[b_addr:.*]] = alloca <4 x float>, align 16
+// AAPCS: %[[p1:.*]] = bitcast %struct.s35* %s1 to <4 x float>*
+// AAPCS: %[[a:.*]] = load <4 x float>, <4 x float>* %[[p1]], align 4
+// AAPCS: %[[p2:.*]] = bitcast %struct.s35* %s2 to <4 x float>*
+// AAPCS: %[[b:.*]] = load <4 x float>, <4 x float>* %[[p2]], align 4
+// AAPCS: store <4 x float> %[[a]], <4 x float>* %[[a_addr]], align 16
+// AAPCS: store <4 x float> %[[b]], <4 x float>* %[[b_addr]], align 16
Index: test/CodeGen/aapcs64-align.cc
===
--- /dev/null
+++ test/CodeGen/aapcs64-align.cc
@@ -0,0 +1,103 @@
+// REQUIRES: arm-registered-target
+// RUN: %clang_cc1 -triple aarch64-none-none-eabi \
+// RUN:   -O2 \
+// RUN:   -emit-llvm -o - %s | FileCheck %s
+
+extern "C" {
+
+// Base case, nothing interesting.
+struct S {
+  long x, y;
+};
+
+void f0(long, S);
+void f0m(long, long, long, long, long, S);
+void g0() {
+  S s = {6, 7};
+  f0(1, s);
+  f0m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g0
+// CHECK: call void @f0(i64 1, [2 x i64] [i64 6, i64 7]
+// CHECK: call void @f0m{{.*}}[2 x i64] [i64 6, i64 7]
+// CHECK: declare void @f0(i64, [2 x i64])
+// CHECK: declare void @f0m(i64, i64, i64, i64, i64, [2 x i64])
+
+// Aligned struct, passed according to its natural alignment.
+struct __attribute__((aligned(16))) S16 {
+  long x, y;
+} s16;
+
+void f1(long, S16);
+void f1m(long, long, long, long, long, S16);
+void g1() {
+  S16 s = {6, 7};
+  f1(1, s);
+  f1m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g1
+// CHECK: call void @f1{{.*}}[2 x i64] [i64 6, i64 7]
+// CHECK: call void @f1m{{.*}}[2 x i64] [i64 6, i64 7]
+// CHECK: declare void @f1(i64, [2 x i64])
+// CHECK: declare void @f1m(i64, i64, i64, i64, i64, [2 x i64])
+
+// Increased natural alignment.
+struct SF16 {
+  long x __attribute__((aligned(16)));
+  long y;
+};
+
+void f3(long, SF16);
+void f3m(long, long, long, long, long, SF16);
+void g3() {
+  SF16 s = {6, 7};
+  f3(1, s);
+  f3m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g3
+// CHECK: call void @f3(i64 1, i128 129127208515966861318)
+// CHECK: call void @f3m(i64 1, i64 2, i64 3, i64 4, i64 5, i128 129127208515966861318)
+// CHECK: declare void @f3(i64, i128)
+// CHECK: declare void @f3m(i64, i64, i64, i64, i64, i128)
+
+
+// Packed structure.
+struct  __attribute__((packed)) P {
+  int x;
+  long u;
+};
+
+void f4(int, P);
+void f4m(int, int, int, int, int, P);
+void g4() {
+  P s = {6, 7};
+  f4(1, s);
+  f4m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g4()
+// CHECK: call void @f4(i32 1, [2 x i64] [i64 30064771078, i64 0])
+// CHECK: void @f4m(i32 1, i32 2, i32 3, i32 4, i32 5, [2 x i64] [i64 30064771078, i64 0])
+// CHECK: declare void @f4(i32, [2 x i64])
+// CHECK: declare void @f4m(i32, i32, i32, i32, i32, [2 x i64])
+
+
+// Packed structure, overaligned, same as above.
+struct  __attribute__((packed, aligned(16))) P16 {
+  int x;
+  long y;
+};
+
+void f5(int, P16);
+void f5m(int, int, int, int, int, P16);
+  void g5() {
+P16 s = {6, 7};
+f5(1, s);
+f5m(1, 2, 3, 4, 5, s);
+}
+// CHECK: define void @g5()
+// CHECK: call void @f5(i32 1, [2 x i64] [i64 30064771078, i64 0])
+// CHECK: void @f5m(i32 1, i32 2, i32 3, i32 4, i32 5, [2 x i64] [i64 30064771078, i64 0])
+// CHECK: declare void @f5(i32, [2 x i64])
+// CHECK: declare void @f5m(i32, i32, i32, i32, i32, [2 x i64])
+
+}

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Ping?


https://reviews.llvm.org/D46013



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

[PATCH] D48916: Fix setting of empty implicit-section-name attribute for functions affected by '#pragma clang section'

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: test/CodeGen/clang-sections-attribute.c:1
+// RUN: %clang_cc1 -emit-llvm -triple arm-none-eabi -o - %s | FileCheck %s
+

Isn't it possible for the test to fail if the Arm target is not configured?


Repository:
  rC Clang

https://reviews.llvm.org/D48916



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

[PATCH] D48916: Fix setting of empty implicit-section-name attribute for functions affected by '#pragma clang section'

[Momchil Velikov via Phabricator via cfe-commits]

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

LGTM. Please, wait a couple of days before committing to give a chance to 
someone else to weigh in.


Repository:
  rC Clang

https://reviews.llvm.org/D48916



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

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Ping?


https://reviews.llvm.org/D46013



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

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Ping?


https://reviews.llvm.org/D46013



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

[PATCH] D62394: [ARM][CMSE] Add CMSE header & builtins

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

I hope I will be able to pick this up in the following weeks and land patches a 
couple of weeks later. Sorry for the delay, but priorities shift all the time ;)


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

https://reviews.llvm.org/D62394



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

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

TBH, I quite dislike  the creeping abuse of `SubtargetFeature`s as code 
generation options.

cf. Target.td:1477

  
//===--===//
  // SubtargetFeature - A characteristic of the chip set.
  //

IMHO, since reserved registes are per-function, this strongly suggests 
implementation as function attribute(s), rather than subtarget features (also 
for the pre-existing r9).
It also opens the path towards possible future `__attribute__`.




Comment at: clang/include/clang/Basic/TargetInfo.h:944
+  /// using the corresponding -ffixed-RegName option.
+  virtual bool isRegisterReservedGlobally(StringRef RegName) const {
+return true;

Parameter name can be omitted if unused; that would remove a potential warning.



Comment at: clang/lib/Basic/Targets/ARM.cpp:884
+StringRef RegName, unsigned RegSize, bool &HasSizeMismatch) const {
+  if (RegName.equals("r6") || RegName.equals("r7") || RegName.equals("r8") ||
+  RegName.equals("r9") || RegName.equals("r10") || RegName.equals("r11") ||

Perhaps you can use here `RegName == "r6"` or string switch ?



Comment at: clang/lib/Basic/Targets/ARM.cpp:890
+  }
+  return false;
+}

`HasSizeMismatch`  is not set along all possible paths.




Comment at: clang/lib/Basic/Targets/ARM.cpp:895
+  // The "sp" register does not have a -ffixed-sp option,
+  // so enable it unconditionally.
+  if (RegName.equals("sp"))

s/enable/reserve/ ?



Comment at: clang/lib/Basic/Targets/ARM.cpp:899
+
+  // enable rN (N:6-11) registers only if the corresponding
+  // +reserve-rN feature is found

Likewise ?



Comment at: clang/lib/Basic/Targets/ARM.cpp:901-902
+  // +reserve-rN feature is found
+  std::vector &Features = getTargetOpts().Features;
+  std::string SearchFeature = "+reserve-" + RegName.str();
+  for (std::string &Feature : Features) {

These variables can be `const`.



Comment at: clang/lib/Basic/Targets/ARM.cpp:903-907
+  for (std::string &Feature : Features) {
+if (Feature.compare(SearchFeature) == 0)
+  return true;
+  }
+  return false;

This explicit loop can be written like:
```
return llvm::any_of(getTargetOpts().Features(),
   [&](auto &P) { return P == SearchFeature; });
```




Comment at: llvm/lib/Target/ARM/ARMSubtarget.h:236
+  // ReservedRRegisters[i] - R#i is not available as a general purpose 
register.
+  BitVector ReservedRRegisters;
 

The usual designation for these registers is "GPR". Suggestion either 
`ReservedGPRegisters` or just `ReservedRegisters`,
here and elsewhere.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68862



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

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/Basic/Targets/ARM.cpp:902
+  std::vector &Features = getTargetOpts().Features;
+  std::string SearchFeature = "+reserve-" + RegName.str();
+  for (std::string &Feature : Features) {

SjoerdMeijer wrote:
> I was pointed at something similar myself recently, but if I am not mistaken 
> then I think this is a use-after-free:
> 
>"+reserve-" + RegName.str()
> 
> this will allocate a temp `std::string` that `SearchFeature` points to, which 
> then gets released, and `SearchFeature` is still pointing at it.
Any temporaries would be destructed at the end of the full expression. By that 
time, the `SearchString` would be constructed and stand on its own.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68862



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

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D68862#1708079 , @carwil wrote:

> > IMHO, since reserved registes are per-function, this strongly suggests 
> > implementation as function attribute(s), rather than subtarget features 
> > (also for the pre-existing r9).
>
> What do you mean reserved registers are per-function? That sounds like you're 
> describing local register variables, which I don't believe Clang has any 
> support for (and there aren't a great deal of use-cases for anyway).
>  We're specifically talking about global usage here.


I mean that the set is dynamically computed and depends on the specific 
function: cf. 
https://github.com/llvm/llvm-project/blob/master/llvm/lib/Target/ARM/ARMBaseRegisterInfo.cpp#L184
as opposed to, say, depend only on target/subtarget/abi.


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

https://reviews.llvm.org/D68862



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

[PATCH] D69297: [ARM][AArch64] Implement __arm_rsrf, __arm_rsrf64, __arm_wsrf & __arm_wsrf64

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

I'm a bit unsure how much we're willing to pollute the namespace: in this 
particular case looking at the four `__bitcast_*` functions.
I appreciate that getting rid of them would  require emitting LLVM IR bitcasts, 
so a bit more effort compared to the macro approach.

Comments?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69297



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

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

I don't think this behaviour is correct with regard to the specification 
(AAELF64 2020Q2):

> Static linkers processing ELF relocatable objects must set the feature bit in 
> the output object or image
>  only if all the input objects have the corresponding feature bit set.



> GNU_PROPERTY_AARCH64_FEATURE_1_BTI This indicates that all executable 
> sections are compatible with
>  Branch Target Identification mechanism. An executable or shared object with 
> this bit set is required to
>  generate Custom PLTs (page 35) with BTI instruction.
> 
> GNU_PROPERTY_AARCH64_FEATURE_1_PAC This indicates that all executable 
> sections have Return Address
>  Signing enabled. An executable or shared object with this bit set can 
> generate Custom PLTs (page 35)
>  with a PAC instruction.

Compatibility of executable sections ultimately depends on each individual 
function, therefore
it cannot be inferred from command-line options alone (transitively from module 
flags), which
merely set a default that can be overridden by function attributes.

If any function has the attribute "sign-return-address", then the output note
section should have PAC bit set. The return address signing is completely local
to the function, and functions with or without return address signing can be
freely mixed with each other.

Likewise, if any function has the attribute "branch-target-enforcement", then
the output note section should have the BTI flag set. Even though code compiled
with BTI is not necessarily compatible with non-BTI code:

- the only way to get BTI code is by explicit use of `-mbranch-protection=...` 
command-line option, or the corresponding attribute, which we should consider a 
clear indication about the user's intent to use BTI.
- the only way to get a mix of present/non-present "branch-target-enforcement" 
attributes is by the explicit use of the 
`__attribute__((target("branch-protection=..."))`, in which case we should 
assume the user knows what they are doing.

What do to if there are no functions in the compile unit?

Technically, objects produced from such a unit are fully compatible with both 
PAC and BTI, which
means both flags should be set. But looking at the (non-existent) function 
attributes alone does
not allow us to unambiguously derive a user's intent to use PAC/BTI. In this 
case, I would suggest
setting the ELF note flags, according to the LLVM IR module flags.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80791



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

[PATCH] D82948: [Driver][ARM] Disable unsupported features when nofp arch extension is used

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:476
+// -mfpu=none, -march=armvX+nofp or -mcpu=X+nofp is *very* similar to
+// -mfloat-abi=soft, only that it should not disable MVE-I.
 Features.insert(Features.end(),

DavidSpickett wrote:
> Why not disable MVE-I? I assume because it's integer only but then why does 
> -mfloat-abi=soft disable it?
> 
> If possible add a regression test for this. In general a test like the bf16 
> test below, but for all the listed extensions would help. Perhaps it makes 
> more sense to add a driver test that looks for the "-" bits in the -### 
> output instead of doing each extension on its own.
> Why not disable MVE-I?

After MVE, "FPU" registers are a separate entity from the FPU.

`-mfpu=none`/`+nofp` disable the FPU. MVE-I does not require an FPU.
`-mfloat-abi=soft` disables both the FPU instructions and the FPU registers.
MVE-I requires "FPU" registers.

It's possible to define different semantics, but this is the one we agreed with 
GCC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82948



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

[PATCH] D75044: [AArch64] __builtin_return_address for PAuth.

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

I'm afraid the patch does not work yet. For example, when the following program

  void *f() {
void g();
g();
return __builtin_return_address(0);
  }

is compiled with

  ./bin/clang -target aarch64-eabi -march=armv8.3-a  
-mbranch-protection=pac-ret -S -O2 h.c

The issue is that the definition of the instructions `XPAC{D,I}` is incorrect: 
it does not mention at all the operand to those insns.
Another problem is that the patch does not work with `-O0`. When compiling 
without optimisations, AArch64 backend used GlobalISel.

I have patches for these two issues. I'll post the one for XPAC{D,O} tomorrow 
and perhaps in a couple of days the GlobalISel one and we're good to go.




Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:6119
+SDValue Reg =
+DAG.getCopyToReg(DAG.getEntryNode(), DL, AArch64::X0, ReturnAddress);
+SDNode *St = DAG.getMachineNode(AArch64::XPACI, DL, VT, Reg);

We shouldn't be hardcoding the `X0` register here.  We already have the encoded 
return address in `ReturnAddress`
can simply do:

   SDNode *St = DAG.getMachineNode(AArch64::XPACI, DL, VT, ReturnAddress);



Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:6124
+// XPACLRI operates on LR therefore we must move the operand accordingly.
+SDValue Reg =
+DAG.getCopyToReg(DAG.getEntryNode(), DL, AArch64::LR, ReturnAddress);

Rename `Reg` to `Chain`.


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

https://reviews.llvm.org/D75044



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

[PATCH] D75044: [AArch64] __builtin_return_address for PAuth.

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D75044#2165496 , @chill wrote:

> The issue is that the definition of the instructions `XPAC{D,I}` is 
> incorrect: it does not mention at all the operand to those insns.


Err, they do mention the operand, but only as an input one, it should be 
input/output.


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

https://reviews.llvm.org/D75044



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

[PATCH] D75044: [AArch64] __builtin_return_address for PAuth.

[Momchil Velikov via Phabricator via cfe-commits]

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

LGTM. Thanks!


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

https://reviews.llvm.org/D75044



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

[PATCH] D82948: [Driver][ARM] Disable unsupported features when nofp arch extension is used

[Momchil Velikov via Phabricator via cfe-commits]

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

LGTM, thanks!




Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:481
+{"-dotprod", "-fp16fml", "-bf16", "-mve.fp"});
+if (!hasIntegerMVE(Features)) {
   Features.emplace_back("-fpregs");

LLVM coding standards call for not using braces on single-statement bodies and 
that's also the style in this source file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82948

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

[PATCH] D75044: [AArch64] __builtin_extract_return_addr for PAuth.

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Needs a test in `clang/test` that `__builtin_extract_return_address` is 
translated to `llvm.extractreturnaddress`.

What if LLVM IR contains a call to `llvm.extractreturnaddress`, but the target 
is not AArch64?




Comment at: llvm/include/llvm/CodeGen/ISDOpcodes.h:74
 /// the parent's frame or return address, and so on.
-FRAMEADDR, RETURNADDR, ADDROFRETURNADDR, SPONENTRY,
+FRAMEADDR, RETURNADDR, ADDROFRETURNADDR, EXTRACTRETURNADDR, SPONENTRY,
 

Needs a comment about `EXTRACTRETURNADDR`. And also a slightly different 
grouping, so the non-commented/undocumented things stand out.


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

https://reviews.llvm.org/D75044



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

[PATCH] D75044: [AArch64] __builtin_extract_return_addr for PAuth.

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D75044#1966997 , @chill wrote:

> Needs a test in `clang/test` that `__builtin_extract_return_address` is 
> translated to `llvm.extractreturnaddress`.


Nevermind, I'm blind.


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

https://reviews.llvm.org/D75044



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

[PATCH] D75044: [AArch64] __builtin_extract_return_addr for PAuth.

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:5151
+  return Address;
+}
+llvm::Function *F =

Can drop the extra braces here.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:5156
+llvm::CallInst *Call = CGF.Builder.CreateCall(F, Address);
+Call->setDoesNotAccessMemory();
+return Call;

Is this necessary, the intrinsic already has `IntrNoMem`?


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

https://reviews.llvm.org/D75044



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

[PATCH] D75181: [AArch64] Handle BTI/PAC in case of generated functions.

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:5149-5152
+  if (BPI.BranchTargetEnforcement)
+Fn->addFnAttr("branch-target-enforcement", "true");
+  else
+Fn->addFnAttr("branch-target-enforcement", "false");

danielkiss wrote:
> I'm going to rebase the patch. I add there a new attribute here 
> "ignore-branch-target-enforcement"
> so then the "branch-target-enforcement"="true"/"false" could be just 
> "branch-target-enforcement".
> 
> 
TBH, that's worse, IMHO.

Ideally, I *think* we'd like *every* LLVM IR function that the backend sees,
regardless of how, why and by whom it is created, to have (or not have)
the three existing PACBTI attributes "sign-return-address", 
"sign-return-address-key", and "branch-target-enforcement", so the backend can 
generate code accordingly.

The module attributes are LLVM IR metadata,  and  AFAIK LLVM IR metadata is an 
optional extra, 
it should not affect correctness.
Indeed, *module* metadata is a somwhat grey area,  better not use it if there a 
way around it.





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

https://reviews.llvm.org/D75181



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

cfe-commits@lists.llvm.org

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: llvm/test/CodeGen/AArch64/aarch64-bf16-ldst-intrinsics.ll:264
+; Function Attrs: argmemonly nounwind readonly
+declare { <8 x bfloat>, <8 x bfloat> } 
@llvm.aarch64.neon.ld2lane.v8bf16.p0i8(<8 x bfloat>, <8 x bfloat>, i64, i8*) #3
+

LukeGeeson wrote:
> SjoerdMeijer wrote:
> > LukeGeeson wrote:
> > > arsenm wrote:
> > > > Why is the IR type name bfloat and not bfloat16?
> > > The naming for the IR type was agreed upon here after quite a big 
> > > discussion. 
> > > https://reviews.llvm.org/D78190
> > I regret very much that I didn't notice this earlier... I.e., I noticed 
> > this in D76077 and wrote that I am relatively unhappy about this (I think I 
> > mentioned this on another ticket too).
> > Because like @arsenm , I would expect the IR type name to be bfloat16.
> > 
> > Correct me if I am wrong, but I don't see a big discussion about this in 
> > D78190. I only see 1 or 2 comments about `BFloat` vs `Bfloat`.
> I cannot see a discussion about the IR type name per-se but I can see you 
> were both involved in the discussion more generally.
> 
> I am concerned that this patch is the wrong place to discuss such issues, and 
> that we should bring this up in a more appropriate place as you mention so 
> that this patch isn't held back.
I don't see a compelling reason for the name to be `bfloat16` or `bfloat3`, 
etc. Like other floating-point types (`float`, `double`, and `half`), the name 
denotes a specific externally defined format, unlike `iN`.


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

https://reviews.llvm.org/D80716



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

cfe-commits@lists.llvm.org

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: llvm/test/CodeGen/AArch64/aarch64-bf16-ldst-intrinsics.ll:264
+; Function Attrs: argmemonly nounwind readonly
+declare { <8 x bfloat>, <8 x bfloat> } 
@llvm.aarch64.neon.ld2lane.v8bf16.p0i8(<8 x bfloat>, <8 x bfloat>, i64, i8*) #3
+

SjoerdMeijer wrote:
> chill wrote:
> > LukeGeeson wrote:
> > > SjoerdMeijer wrote:
> > > > LukeGeeson wrote:
> > > > > arsenm wrote:
> > > > > > Why is the IR type name bfloat and not bfloat16?
> > > > > The naming for the IR type was agreed upon here after quite a big 
> > > > > discussion. 
> > > > > https://reviews.llvm.org/D78190
> > > > I regret very much that I didn't notice this earlier... I.e., I noticed 
> > > > this in D76077 and wrote that I am relatively unhappy about this (I 
> > > > think I mentioned this on another ticket too).
> > > > Because like @arsenm , I would expect the IR type name to be bfloat16.
> > > > 
> > > > Correct me if I am wrong, but I don't see a big discussion about this 
> > > > in D78190. I only see 1 or 2 comments about `BFloat` vs `Bfloat`.
> > > I cannot see a discussion about the IR type name per-se but I can see you 
> > > were both involved in the discussion more generally.
> > > 
> > > I am concerned that this patch is the wrong place to discuss such issues, 
> > > and that we should bring this up in a more appropriate place as you 
> > > mention so that this patch isn't held back.
> > I don't see a compelling reason for the name to be `bfloat16` or `bfloat3`, 
> > etc. Like other floating-point types (`float`, `double`, and `half`), the 
> > name denotes a specific externally defined format, unlike `iN`.
> > Like other floating-point types (float, double, and half), the name denotes 
> > a specific externally defined format, 
> 
> Is the defined format not called bfloat16?
Indeed, people use the name "bfloat16". But then the `half`, `float`, and 
`double` also differ from the official `binary16`, `binarty32`, and `binary64`.
IMHO `bfloat` fits better in the LLVM IR naming convention.


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

https://reviews.llvm.org/D80716



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

cfe-commits@lists.llvm.org

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: llvm/test/CodeGen/AArch64/aarch64-bf16-ldst-intrinsics.ll:264
+; Function Attrs: argmemonly nounwind readonly
+declare { <8 x bfloat>, <8 x bfloat> } 
@llvm.aarch64.neon.ld2lane.v8bf16.p0i8(<8 x bfloat>, <8 x bfloat>, i64, i8*) #3
+

SjoerdMeijer wrote:
> chill wrote:
> > SjoerdMeijer wrote:
> > > chill wrote:
> > > > LukeGeeson wrote:
> > > > > SjoerdMeijer wrote:
> > > > > > LukeGeeson wrote:
> > > > > > > arsenm wrote:
> > > > > > > > Why is the IR type name bfloat and not bfloat16?
> > > > > > > The naming for the IR type was agreed upon here after quite a big 
> > > > > > > discussion. 
> > > > > > > https://reviews.llvm.org/D78190
> > > > > > I regret very much that I didn't notice this earlier... I.e., I 
> > > > > > noticed this in D76077 and wrote that I am relatively unhappy about 
> > > > > > this (I think I mentioned this on another ticket too).
> > > > > > Because like @arsenm , I would expect the IR type name to be 
> > > > > > bfloat16.
> > > > > > 
> > > > > > Correct me if I am wrong, but I don't see a big discussion about 
> > > > > > this in D78190. I only see 1 or 2 comments about `BFloat` vs 
> > > > > > `Bfloat`.
> > > > > I cannot see a discussion about the IR type name per-se but I can see 
> > > > > you were both involved in the discussion more generally.
> > > > > 
> > > > > I am concerned that this patch is the wrong place to discuss such 
> > > > > issues, and that we should bring this up in a more appropriate place 
> > > > > as you mention so that this patch isn't held back.
> > > > I don't see a compelling reason for the name to be `bfloat16` or 
> > > > `bfloat3`, etc. Like other floating-point types (`float`, `double`, and 
> > > > `half`), the name denotes a specific externally defined format, unlike 
> > > > `iN`.
> > > > Like other floating-point types (float, double, and half), the name 
> > > > denotes a specific externally defined format, 
> > > 
> > > Is the defined format not called bfloat16?
> > Indeed, people use the name "bfloat16". But then the `half`, `float`, and 
> > `double` also differ from the official `binary16`, `binarty32`, and 
> > `binary64`.
> > IMHO `bfloat` fits better in the LLVM IR naming convention.
> yeah, so that's exactly why I don't follow your logic. If there's any logic 
> in the names here, the mapping from source-language type to IR type seems the 
> most plausible one. And I just don't see the benefit of dropping the 16, and 
> how that would fit better in some naming scheme or how that makes things 
> clearer here.
What source language?

That said, I'm resigning from the bikeshedding here.


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

https://reviews.llvm.org/D80716



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

[PATCH] D81837: [ARM][bfloat] Removing lowering of bfloat arguments and returns from Clang's CodeGen

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Pretty straightforward, LGTM. I'd suggest rewording the title (presumably 
commit message summary) into something like "Do not coerce bfloat arguments and 
returns to integers", as we're obviously still lowering C and C++ to LLVM LR.§§


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81837



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

[PATCH] D82948: [Driver][ARM] Disable unsupported features when nofp arch extension is used

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Needs a regression test. This patch and the dependent patch clash, better with 
a single patch.




Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:288
 
+static void appendNoFPUnsupportedFeatures(const arm::FloatABI ABI,
+  const unsigned FPUID,

That's kinda mouthful name.



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:292-297
+  auto checkFPDisabledInArchName = [](const StringRef &ArchName) {
+SmallVector Split;
+ArchName.split(Split, '+', -1, false);
+return llvm::any_of(
+Split, [](const StringRef &Extension) { return Extension == "nofp"; });
+  };

Wouldn't just looking for the substring do the job?

Also need to handle `-mcpu=...+nofp`.

We already "parse" the arguments to `-march=` and `-mcpu=` (and `-mfpu=`) 
earlier, it seems to me we
could note the `+nofp` and `+nofp.dp` earlier. (TBH, it isn't immediately 
obvious to me how to untangle this mess).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82948



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

[PATCH] D82948: [Driver][ARM] Disable unsupported features when nofp arch extension is used

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:292-297
+  auto checkFPDisabledInArchName = [](const StringRef &ArchName) {
+SmallVector Split;
+ArchName.split(Split, '+', -1, false);
+return llvm::any_of(
+Split, [](const StringRef &Extension) { return Extension == "nofp"; });
+  };

chill wrote:
> Wouldn't just looking for the substring do the job?
> 
> Also need to handle `-mcpu=...+nofp`.
> 
> We already "parse" the arguments to `-march=` and `-mcpu=` (and `-mfpu=`) 
> earlier, it seems to me we
> could note the `+nofp` and `+nofp.dp` earlier. (TBH, it isn't immediately 
> obvious to me how to untangle this mess).
> 
Hmm, actually, `+nofp.dp` should not disable the FPU, I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82948



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

[PATCH] D82948: [Driver][ARM] Disable unsupported features when nofp arch extension is used

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:292-297
+  auto checkFPDisabledInArchName = [](const StringRef &ArchName) {
+SmallVector Split;
+ArchName.split(Split, '+', -1, false);
+return llvm::any_of(
+Split, [](const StringRef &Extension) { return Extension == "nofp"; });
+  };

vhscampos wrote:
> vhscampos wrote:
> > chill wrote:
> > > chill wrote:
> > > > Wouldn't just looking for the substring do the job?
> > > > 
> > > > Also need to handle `-mcpu=...+nofp`.
> > > > 
> > > > We already "parse" the arguments to `-march=` and `-mcpu=` (and 
> > > > `-mfpu=`) earlier, it seems to me we
> > > > could note the `+nofp` and `+nofp.dp` earlier. (TBH, it isn't 
> > > > immediately obvious to me how to untangle this mess).
> > > > 
> > > Hmm, actually, `+nofp.dp` should not disable the FPU, I think.
> > Just looking for the substring might be sufficient indeed.
> > 
> > Yes, we already do `-march`/`-mcpu` parsing a bit earlier. However, this 
> > parsing and the following handling of it is done deeper in the call stack. 
> > I wondered about ways to propagate this information back to this point here 
> > (e.g. adding one more by-ref argument that is set by the first round of 
> > parsing), but I don't feel confident to back it up.
> > 
> > Are you okay with me just changing it to a substring search?
> Actually it may be better to keep the string splitting method. The search 
> required here must be whole-word, as to flag up "+nofp", but not "+nofp.dp". 
> It can be done with less code using the current list of tokens as opposed to 
> using substring search, followed by a "is it whole-word?" check.
In fact, it's less number of tokens, not that number of tokens is that 
important.

auto anyNOFP = [](const StringRef &str) {
  size_t pos = str.find_lower("+nofp");
  return pos != StringRef::npos &&
  (pos + 5 == str.size() || str[pos + 5] == '+');
};

Or it could become

auto findFeature = [](const StringRef &str, const StringRef &feature) {
  size_t pos = str.find_lower(feature);
  return (pos == StringRef::npos || pos + feature.size() == str.size() ||
  str[pos + feature.size()] == '+')
 ? pos
 : StringRef::npos;
};

which is slightly longer, but more universal and allows you to check relative 
positions of features.

A-a-a-nyway, I think this string twiddling should be the absolute last resort. 

Could you please, see, if `checkARMArchName` and `checkARMCpuName` can be 
tweaked into
returning `llvm::ARM::FK_NONE` or `llvm::ARM::FK_INVALID` or whatever, 
depending on what is present
in option value of `-march=...` and `-mcpu=...`?

An interesting question is what to do if there are contradictions, but I think 
the general strategy is to not strive to produce sensible output from 
nonsensical input.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82948



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

[PATCH] D77270: Fix the check for regparm in FunctionType::ExtInfo

[Momchil Velikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG334ac8105401: Fix the check for regparm in 
FunctionType::ExtInfo (authored by chill).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77270

Files:
  clang/include/clang/AST/Type.h
  clang/test/AST/spurious-regparm.c


Index: clang/test/AST/spurious-regparm.c
===
--- /dev/null
+++ clang/test/AST/spurious-regparm.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -triple armv8.1m.main-eabi -mcmse -fsyntax-only %s 
-ast-dump | FileCheck %s
+// REQUIRES: arm-registered-target
+typedef int (*fn_t)(int) __attribute__((cmse_nonsecure_call));
+// CHECK-NOT: regparm 0
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -3518,13 +3518,12 @@
 enum { NoReturnMask = 0x20 };
 enum { ProducesResultMask = 0x40 };
 enum { NoCallerSavedRegsMask = 0x80 };
-enum { NoCfCheckMask = 0x800 };
-enum { CmseNSCallMask = 0x1000 };
 enum {
-  RegParmMask = ~(CallConvMask | NoReturnMask | ProducesResultMask |
-  NoCallerSavedRegsMask | NoCfCheckMask | CmseNSCallMask),
+  RegParmMask =  0x700,
   RegParmOffset = 8
-}; // Assumed to be the last field
+};
+enum { NoCfCheckMask = 0x800 };
+enum { CmseNSCallMask = 0x1000 };
 uint16_t Bits = CC_C;
 
 ExtInfo(unsigned Bits) : Bits(static_cast(Bits)) {}
@@ -3557,7 +3556,7 @@
 bool getCmseNSCall() const { return Bits & CmseNSCallMask; }
 bool getNoCallerSavedRegs() const { return Bits & NoCallerSavedRegsMask; }
 bool getNoCfCheck() const { return Bits & NoCfCheckMask; }
-bool getHasRegParm() const { return (Bits >> RegParmOffset) != 0; }
+bool getHasRegParm() const { return ((Bits & RegParmMask) >> 
RegParmOffset) != 0; }
 
 unsigned getRegParm() const {
   unsigned RegParm = (Bits & RegParmMask) >> RegParmOffset;


Index: clang/test/AST/spurious-regparm.c
===
--- /dev/null
+++ clang/test/AST/spurious-regparm.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -triple armv8.1m.main-eabi -mcmse -fsyntax-only %s -ast-dump | FileCheck %s
+// REQUIRES: arm-registered-target
+typedef int (*fn_t)(int) __attribute__((cmse_nonsecure_call));
+// CHECK-NOT: regparm 0
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -3518,13 +3518,12 @@
 enum { NoReturnMask = 0x20 };
 enum { ProducesResultMask = 0x40 };
 enum { NoCallerSavedRegsMask = 0x80 };
-enum { NoCfCheckMask = 0x800 };
-enum { CmseNSCallMask = 0x1000 };
 enum {
-  RegParmMask = ~(CallConvMask | NoReturnMask | ProducesResultMask |
-  NoCallerSavedRegsMask | NoCfCheckMask | CmseNSCallMask),
+  RegParmMask =  0x700,
   RegParmOffset = 8
-}; // Assumed to be the last field
+};
+enum { NoCfCheckMask = 0x800 };
+enum { CmseNSCallMask = 0x1000 };
 uint16_t Bits = CC_C;
 
 ExtInfo(unsigned Bits) : Bits(static_cast(Bits)) {}
@@ -3557,7 +3556,7 @@
 bool getCmseNSCall() const { return Bits & CmseNSCallMask; }
 bool getNoCallerSavedRegs() const { return Bits & NoCallerSavedRegsMask; }
 bool getNoCfCheck() const { return Bits & NoCfCheckMask; }
-bool getHasRegParm() const { return (Bits >> RegParmOffset) != 0; }
+bool getHasRegParm() const { return ((Bits & RegParmMask) >> RegParmOffset) != 0; }
 
 unsigned getRegParm() const {
   unsigned RegParm = (Bits & RegParmMask) >> RegParmOffset;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76369: [CMSE] Clear padding bits of struct/unions/fp16 passed by value

[Momchil Velikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG102b4105e3fd: [CMSE] Clear padding bits of 
struct/unions/fp16 passed by value (authored by chill).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76369

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CodeGen/cmse-clear-arg.c
  clang/test/CodeGen/cmse-clear-fp16.c
  clang/test/CodeGen/cmse-clear-return.c
  clang/test/Sema/arm-cmse-no-diag.c
  clang/test/Sema/arm-cmse.c

Index: clang/test/Sema/arm-cmse.c
===
--- clang/test/Sema/arm-cmse.c
+++ clang/test/Sema/arm-cmse.c
@@ -28,3 +28,30 @@
 void fn1() __attribute__((cmse_nonsecure_entry(1)));  // expected-error {{'cmse_nonsecure_entry' attribute takes no arguments}}
 
 typedef void (*fn2_t)() __attribute__((cmse_nonsecure_call("abc"))); // expected-error {{'cmse_nonsecure_call' attribute takes no argument}}
+
+union U { unsigned n; char b[4]; } u;
+
+union U xyzzy() __attribute__((cmse_nonsecure_entry)) {
+  return u; // expected-warning {{passing union across security boundary via return value may leak information}}
+}
+
+void (*fn2)(int, union U) __attribute__((cmse_nonsecure_call));
+void (*fn3)() __attribute__ ((cmse_nonsecure_call));
+
+struct S {
+  int t;
+  union {
+char b[4];
+unsigned w;
+  };
+} s;
+
+void qux() {
+  fn2(1,
+  u); // expected-warning {{passing union across security boundary via parameter 1 may leak information}}
+
+  fn3(
+   u, // expected-warning {{passing union across security boundary via parameter 0 may leak information}}
+   1,
+   s); // expected-warning {{passing union across security boundary via parameter 2 may leak information}}
+}
Index: clang/test/Sema/arm-cmse-no-diag.c
===
--- /dev/null
+++ clang/test/Sema/arm-cmse-no-diag.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -triple thumbv8m.base-none-eabi -mcmse -verify -Wno-cmse-union-leak %s
+// expected-no-diagnostics
+
+union U { unsigned n; char b[4]; } u;
+
+void (*fn2)(int, union U) __attribute__((cmse_nonsecure_call));
+
+union U xyzzy() __attribute__((cmse_nonsecure_entry)) {
+  fn2(0, u);
+  return u;
+}
Index: clang/test/CodeGen/cmse-clear-return.c
===
--- /dev/null
+++ clang/test/CodeGen/cmse-clear-return.c
@@ -0,0 +1,265 @@
+// RUN: %clang_cc1 -triple thumbv8m.main   -O0 -mcmse -S -emit-llvm %s -o - | \
+// RUN:FileCheck %s --check-prefixes=CHECK,CHECK-LE,CHECK-LE-NOPT,CHECK-SOFT
+// RUN: %clang_cc1 -triple thumbebv8m.main -O0 -mcmse -S -emit-llvm %s -o - | \
+// RUN:FileCheck %s --check-prefixes=CHECK,CHECK-BE,CHECK-BE-NOPT,CHECK-SOFT
+// RUN: %clang_cc1 -triple thumbv8m.main   -O2 -mcmse -S -emit-llvm %s -o - | \
+// RUN:FileCheck %s --check-prefixes=CHECK,CHECK-LE,CHECK-LE-OPT,CHECK-SOFT
+// RUN: %clang_cc1 -triple thumbebv8m.main -O2 -mcmse -S -emit-llvm %s -o - | \
+// RUN:FileCheck %s --check-prefixes=CHECK,CHECK-BE,CHECK-BE-OPT,CHECK-SOFT
+// RUN: %clang_cc1 -triple thumbv8m.main   -O0 -mcmse -S -emit-llvm %s -o - \
+// RUN:-mfloat-abi hard | \
+// RUN:FileCheck %s --check-prefixes=CHECK,CHECK-LE,CHECK-LE-NOPT,CHECK-HARD
+
+
+//   :Memory layout| Mask
+// LE: ...1    | 0x0001/1
+// BE: 1...    | 0x8000/-2147483648
+typedef struct T0 {
+  int a : 1, : 31;
+} T0;
+
+T0 t0;
+__attribute__((cmse_nonsecure_entry)) T0 f0() { return t0; }
+// CHECK:define {{.*}} @f0()
+// CHECK-LE: %[[R:.*]] = and i32 %{{.*}}, 1
+// CHECK-BE: %[[R:.*]] = and i32 %{{.*}}, -2147483648
+// CHECK:ret i32 %[[R]]
+
+// LE: ..1.    0x0002/2
+// BE: .1..    0x4000/1073741824
+typedef struct T1 {
+  int : 1, a : 1, : 30;
+} T1;
+
+T1 t1;
+__attribute__((cmse_nonsecure_entry)) T1 f1() { return t1; }
+// CHECK:define {{.*}} @f1()
+// CHECK-LE: %[[R:.*]] = and i32 %{{.*}}, 2
+// CHECK-BE: %[[R:.*]] = and i32 %{{.*}}, 1073741824
+// CHECK:ret i32 %[[R]]
+
+// LE:  ...1   0x0100/256
+// BE:  1...   0x0080/8388608
+typedef struct T2 {
+  int : 8, a : 1, : 23;
+} T2;
+
+T2 t2;
+__attribute__((cmse_nonsecure_entry)) T2 f2() { return t2; }
+// CHECK:define {{.*}} @f2()
+// CHECK-LE: %[[R:.*]] = and i32 %{{.*}}, 256
+// CHECK-BE: %[[R:.*]] = and i32 %{{.*}}, 8388608
+// CHECK:ret i32 %[[R]]
+
+// LE:  .1..   0x0

[PATCH] D78129: Add Marvell ThunderX3T110 support

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.td:849-857
+// These pointer authentication instructions require armv8.3a
+let Predicates = [HasV8_3a, HasPA] in {
 let Uses = [LR], Defs = [LR] in {
   def PACIAZ   : SystemNoOperands<0b000, "hint\t#24">;
   def PACIBZ   : SystemNoOperands<0b010, "hint\t#26">;
   let isAuthenticated = 1 in {
 def AUTIAZ   : SystemNoOperands<0b100, "hint\t#28">;

wxz2020 wrote:
> wxz2020 wrote:
> > ktkachov wrote:
> > > IIRC these instructions are deliberately allowed in pre-armv8.3 targets 
> > > because they are encoded in the NOP-space and can be deployed on 
> > > pre-armv8.3 targets 
> > I will do some research on this.
> According to the documents, pointer authenticatoin got officially supporoted 
> starting from armv8.3.  
These instructions are executed as NOP on pre v8.3-A architectures. It allows 
you to have a single compatible binary that works correctly on pre v8.3-a (ofc, 
without pointer authentication), as well as on
8.3-a and later cores, with pointer authentication.

Please, remove the predicates.


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

https://reviews.llvm.org/D78129



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

[PATCH] D75044: [AArch64] __builtin_return_address for PAuth.

[Momchil Velikov via Phabricator via cfe-commits]

chill requested changes to this revision.
chill added a comment.
This revision now requires changes to proceed.

Let's postpone this just for a little bit, to settle on an approach to `depth  
> 0`.


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

https://reviews.llvm.org/D75044

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

[PATCH] D75044: [AArch64] __builtin_return_address for PAuth.

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D75044#2186973 , @chill wrote:

> Let's postpone this just for a little bit, to settle on an approach to `depth 
>  > 0`.

This is with regard to https://reviews.llvm.org/D84502#inline-779900


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

https://reviews.llvm.org/D75044

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

[PATCH] D82949: [Driver][ARM] Disable bf16 when hardware FP support is missing

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Is this patch needed anymore?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82949

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

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D80791#2164543 , @danielkiss wrote:

>> If any function has the attribute "sign-return-address", then the output note
>> section should have PAC bit set. The return address signing is completely 
>> local
>> to the function, and functions with or without return address signing can be
>> freely mixed with each other.
>
> That is true PAC and non-PAC functions can be mixed. 
> Does one function makes the "all executable sections" pac-ret enabled?

Yes, the presence of even a single function is a clear indication of what the 
user whats - enable PAC/BTI.
The default is not having PAC/BTI code gen, therefore its presence is a result 
of a deliberate action by the user, 
therefore unambiguously conveys the user's intent.

> BTW `GNU_PROPERTY_AARCH64_FEATURE_1_PAC` is not really used for anything.

I may not be used today in GNU/Linux, but still, it has to have sensible 
semantics.

> One of the reasons of the introduction of these macros is the management of 
> the function attributes.
> For example:
>
>   #ifdef __ARM_FEATURE_PAC_DEFAULT
>   #ifdef __ARM_FEATURE_BTI_DEFAULT
>   #define NO_PAC_FUNC __attribute__((target("branch-protection=bti")))
>   #else
>   #define NO_PAC_FUNC __attribute__((target("branch-protection=none")))
>   #endif /* __ARM_FEATURE_BTI_DEFAULT */
>   ...

I don't see how this example is relevant to the discussion of what notes to 
emit.
Our starting point is we have some default state (in module flags or whatever), 
some
individual function state and we have to decide what notes to emit, 
//regardless of the specific way
we came up with those function attributes.//

> In my humble opinion the function attribute is there to alter global setting.
> I considered to propagate the function attribute to the module flags but 
> that would lead to inconsistent compilation with the macros that I'd avoid.

The compilation of a single given function does not necessarily need to be
consistent with the value of these macros. Quite the opposite really, the 
macros themselves are
suffixed by `_DEFAULT` in order to explicitly acknowledge that possibility.

>> What do to if there are no functions in the compile unit?
>>
>> Technically, objects produced from such a unit are fully compatible with 
>> both PAC and BTI, which
>> means both flags should be set. But looking at the (non-existent) function 
>> attributes alone does
>> not allow us to unambiguously derive a user's intent to use PAC/BTI. In this 
>> case, I would suggest
>> setting the ELF note flags, according to the LLVM IR module flags.
>
> I think the only clear indication from the user to use PAC/BTI is the 
> explicit use of `-mbranch-protection=...` command-line option.

Using the attribute is no less clear and even carries more weight, as it 
overrides the command line option.

> A few function attributes that would turn PAC/BTI on just on those few 
> functions makes no sense for me in any real world application.

Turning on/off PAC/BTI is completely symmetrical - one can achieve exactly the 
same effect with:

- command-line options enabling PAC/BTI and individual attributes disabling BTI
- command-line options disabling PAC/BIT (e.g. not having a command-line option 
at all) and individual attributes enabling it

We shouldn't be guessing and prescribing how applications should use the 
mechanisms we make available and certainly
shouldn't be judging what is a real-world application and what is not.

> Valid to turn off PAC/BTI on selected functions while the whole application 
> compiled with them.
>
> We need to turn PAC off on the code path where we change\manage the keys for 
> example.
> Exaggerated example for BTI: https://godbolt.org/z/Y9bhe9  Current version of 
> llvm issues a warning and won't emit the note while I think it should.

Just as valid is to turn on PAC/BTI on selected functions, while the while 
compilation unit (*not* application) is compiled without them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80791

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

[PATCH] D75181: [AArch64] Handle BTI/PAC in case of generated functions.

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

This approach looks way too hackish to me with multiple opposing attributes 
("sign-return-address" vs. "ignore-sign-return-address")
and some convoluted logic to resolve the contradiction.


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

https://reviews.llvm.org/D75181

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

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:5550
+auto &VMContext = CGM.getLLVMContext();
+M->addModuleFlag(llvm::Module::Override, "sign-return-address",
+ Scope == LangOptions::SignReturnAddressScopeKind::All

Wouldn't that cause the sanitiser functions to be also compiled with PAC/BTI?  
(re: D75181)


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

https://reviews.llvm.org/D80791

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

[PATCH] D75181: [AArch64] Handle BTI/PAC in case of generated functions.

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D75181#2193447 , @danielkiss wrote:

> I don't see any other alternative option, I'm open to any other idea.

My original idea was to pass options to LLVM. I'll come up with a patch in a 
day or two (if it works) and then we'll see.


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

https://reviews.llvm.org/D75181

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

[PATCH] D81930: [AArch64] Add -mmark-bti-property flag.

[Momchil Velikov via Phabricator via cfe-commits]

chill requested changes to this revision.
chill added inline comments.
This revision now requires changes to proceed.
Herald added a subscriber: dang.



Comment at: llvm/lib/Target/AArch64/AArch64.td:352
 
+def FeatureEmitNoteBTIProperty : SubtargetFeature<"markbtiproperty", 
"MarkBTIProperty",
+"true", "Emit .note.gnu.property for Branch Target Identification" >;

No, this is an abuse of subtarget features. Subtarget features represent 
characteristics of the chip, they shouldn't be used to pass arbitrary bits of 
information.
Possible alternatives - `TargetOptions` (cf. 
`BackendUtil.cpp:initTargetOptions()`) or
LLVM command-line arguments (cf. `BackendUtil.cpp:setCommandLineOpts()`.


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

https://reviews.llvm.org/D81930

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

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D80791#2196598 , @nsz wrote:

> the assumption is that the intended branch protection is implied via cmdline 
> flags for the tu and function attributes are only used in source code for 
> some hack.

I don't share this assumption. I find it just as valid to control the PAC/BTI 
with things like:

  #ifdef ENABLE_BTI
  #define BTI_FUNC __attribute__((target("branch-protection=bti")))
  #else
  #define BTI_FUNC
  
  BTI_FUNC void foo() { ...
  BTI_FUNC int bar() { ...

without using any command-line option other than `-DENABLE_BTI=1`.


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

https://reviews.llvm.org/D80791

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

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D80791#2206853 , @nsz wrote:

> i think that cannot work.
>
> the implementation is free to inject arbitrary code into
> user code so if the user does not tell the implementation
> that it wants the entire tu to be bti safe then non-bti
> code can end up in there. (e.g. ctor of an instrumentation
> that is not realated to any particular function with the
> bti marking)

Certainly, there are cases it won't work, but there are definitely
cases where it *can* work. Whatever the implementation does
should be a deterministic consequence of implementing the relevant
language standards together with implementation-defined behaviour,
command-line options and language extensions (e..g attributes).

Certainly I don't expect C++ ctorts/dtors in C code and gcov or
sanitiser calls if I haven't given relevant 
`-fprofile-whatever`/`-fsanitize=whatever`
options. In that sense, the implementation cannot do whatever
it pleases, it is constrained to a range of behaviours one can reason about.


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

https://reviews.llvm.org/D80791

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

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

I would prefer to avoid the situation where the markings of two otherwise 
identical files were different,
depending on how the files were produced, no matter if it was a common or a 
special case.


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

https://reviews.llvm.org/D80791

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

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D80791#2209624 , @nsz wrote:

> In D80791#2207203 , @chill wrote:
>
>> I would prefer to avoid the situation where the markings of two otherwise 
>> identical files were different,
>> depending on how the files were produced, no matter if it was a common or a 
>> special case.
>
> i don't see why it is desirable to silently get marking on an object file if 
> function definitions happen to be bti compatible in it:

It is desirable to get the marking because BTI-compatible functions don't 
appear by accident - they are a result of deliberate user actions, which clearly
express intent to use BTI.

> - compiler cannot reliably do this (e.g. bti incompatible inline asm).

Like for any other case, that's entirely the responsibility of the user if they 
use inline asm; Command-line options are not
special with regard to inline asm, so everything that can break 
attribute-derived marking, breaks command-line derived marking.

> - some users don't want the marking: not all linkers support it so it can 
> cause unexpected breakage.

Those linkers would need to be upgraded if the compiler imposes extra 
requirements on them.  One can't hold the compiler hostage to obsolete linkers. 
If users insist, they can just remove the .note section.

> - most users (all?) want the marking reliably (not opportunistically), but 
> function annotations are fragile (can depend on optimizations and code 
> outside of user control).

The user explicitly marking an object is the least reliable option, because 
it's done without regard what the object in actually contains.

> - it is not useful to have a bti annotated function unless everything else is 
> bti compatible too: it is all or nothing per elf module.

This is false. Some functions in an elf module could be in a guarded region, 
some in a non-guarded region. Some function may always
be called in a "BTI-safe" way, which may be unknown to the compiler.

> - but a compiler cannot diagnose if only some functions have the annotation 
> (we don't have a cflag for it) so even if the compiler tried to add the 
> marking silently users cannot rely on it: it's too easy to drop the marking 
> and no way to debug such failure.

At the time a compiler decides to or decides not to emit instructions which 
implement PAC-RET or BTI is perfectly clear what;s the effective annotation for 
each individual function.

I don't really understand the point of all these objections.

With my proposal to derive marking from function attributes, as well as from 
command-line
everything above will still work in the (arguably) most common case that we 
expect - users just using
command line.

I'm proposing to be strict and cover a few corner case where the command-line 
only approach produces bogus results.


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

https://reviews.llvm.org/D80791

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

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D80791#2210124 , @danielkiss wrote:

>>> it is not useful to have a bti annotated function unless everything else is 
>>> bti compatible too: it is all or nothing per elf module.
>>
>> This is false. Some functions in an elf module could be in a guarded region, 
>> some in a non-guarded region. Some function may always
>> be called in a "BTI-safe" way, which may be unknown to the compiler.
>
> Right now the elf and all of the `text` sections considered BTI enabled or 
> not. The dynamic linkers/loaders can't support this
> use case without additional information to be encoded somewhere (and 
> specified). To support such we need to consider grouping/align to page
> boundaries these functions in the linker because BTI could be controlled by 
> flags in PTE.
> With the current spec this usecase is not supported in this way. The user 
> have to link the BTI protected code into another elf.
> Side note: The `force-bti` linker option can't work with half BTI enabled 
> objects.

I suppose this is valid for typical Linux-based systems today.

Is it valid in general, across the whole spectre of operating systems or for 
bare-metal targets?

Guess not.


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

https://reviews.llvm.org/D80791

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

[PATCH] D79693: [test][ARM][CMSE] Use -ffreestanding for arm_cmse.h tests

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

I'm sorry, I don't understand the issue. Certainly it's the compiler (driver) 
responsibility to setup include paths according to the selected target.
How do you trigger a problem?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79693



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

[PATCH] D79693: [test][ARM][CMSE] Use -ffreestanding for arm_cmse.h tests

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

I see.

I can also count (`grep -rn '#include.*https://reviews.llvm.org/D79693/new/

https://reviews.llvm.org/D79693



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

[PATCH] D79693: [test][ARM][CMSE] Use clang_cc1 in arm_cmse.h tests

[Momchil Velikov via Phabricator via cfe-commits]

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

LGTM, thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79693



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

[PATCH] D123498: [clang] Adding Platform/Architecture Specific Resource Header Installation Targets

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Should these lists contain only source tree headers or also generated header 
files? I'm not seeing `arm_mve.h`, for example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123498

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

[PATCH] D119166: [clang][ARM] Re-word PACBTI warning.

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:149
+def warn_incompatible_branch_protection_option: Warning <
+  "'-mbranch-protection=' option incompatible with the '%0' architecture">,
   InGroup;

"is incompatible"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119166

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

[PATCH] D119724: Fix test failure for targets with varying uwtable defaults

[Momchil Velikov via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa31d00ddceb0: Fix test failure for targets with varying 
uwtable defaults (authored by chill).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119724

Files:
  clang/test/CodeGen/uwtable-attr.c


Index: clang/test/CodeGen/uwtable-attr.c
===
--- clang/test/CodeGen/uwtable-attr.c
+++ clang/test/CodeGen/uwtable-attr.c
@@ -1,13 +1,15 @@
 // Test that function and modules attributes react on the command-line options,
 // it does not state the current behaviour makes sense in all cases (it does 
not).
 
-// RUN: %clang -S -emit-llvm -o - %s   
 | FileCheck %s -check-prefixes=CHECK,DEFAULT
-// RUN: %clang -S -emit-llvm -o - %s -funwind-tables
-fno-asynchronous-unwind-tables | FileCheck %s -check-prefixes=CHECK,TABLES
-// RUN: %clang -S -emit-llvm -o - %s -fno-unwind-tables 
-fno-asynchronous-unwind-tables | FileCheck %s -check-prefixes=CHECK,NO_TABLES
+// RUN: %clang -target x86_64-linux -S -emit-llvm -o - %s  
  | FileCheck %s -check-prefixes=CHECK,DEFAULT
+// RUN: %clang -target x86_64-linux -S -emit-llvm -o - %s -funwind-tables
-fno-asynchronous-unwind-tables | FileCheck %s -check-prefixes=CHECK,TABLES
+// RUN: %clang -target x86_64-linux -S -emit-llvm -o - %s -fno-unwind-tables 
-fno-asynchronous-unwind-tables | FileCheck %s -check-prefixes=CHECK,NO_TABLES
 
-// RUN: %clang -S -emit-llvm -o - -x c++ %s
 | FileCheck %s -check-prefixes=CHECK,DEFAULT
-// RUN: %clang -S -emit-llvm -o - -x c++ %s  -funwind-tables   
 -fno-asynchronous-unwind-tables | FileCheck %s -check-prefixes=CHECK,TABLES
-// RUN: %clang -S -emit-llvm -o - -x c++ %s  -fno-exceptions 
-fno-unwind-tables -fno-asynchronous-unwind-tables | FileCheck %s 
-check-prefixes=CHECK,NO_TABLES
+// RUN: %clang -target x86_64-linux -S -emit-llvm -o - -x c++ %s   
  | FileCheck %s 
-check-prefixes=CHECK,DEFAULT
+// RUN: %clang -target x86_64-linux -S -emit-llvm -o - -x c++ %s   
   -funwind-tables-fno-asynchronous-unwind-tables | FileCheck %s 
-check-prefixes=CHECK,TABLES
+// RUN: %clang -target x86_64-linux -S -emit-llvm -o - -x c++ %s  
-fno-exceptions -fno-unwind-tables -fno-asynchronous-unwind-tables | FileCheck 
%s -check-prefixes=CHECK,NO_TABLES
+
+// REQUIRES: x86-registered-target
 
 #ifdef __cplusplus
 extern "C" void g(void);


Index: clang/test/CodeGen/uwtable-attr.c
===
--- clang/test/CodeGen/uwtable-attr.c
+++ clang/test/CodeGen/uwtable-attr.c
@@ -1,13 +1,15 @@
 // Test that function and modules attributes react on the command-line options,
 // it does not state the current behaviour makes sense in all cases (it does not).
 
-// RUN: %clang -S -emit-llvm -o - %s| FileCheck %s -check-prefixes=CHECK,DEFAULT
-// RUN: %clang -S -emit-llvm -o - %s -funwind-tables-fno-asynchronous-unwind-tables | FileCheck %s -check-prefixes=CHECK,TABLES
-// RUN: %clang -S -emit-llvm -o - %s -fno-unwind-tables -fno-asynchronous-unwind-tables | FileCheck %s -check-prefixes=CHECK,NO_TABLES
+// RUN: %clang -target x86_64-linux -S -emit-llvm -o - %s| FileCheck %s -check-prefixes=CHECK,DEFAULT
+// RUN: %clang -target x86_64-linux -S -emit-llvm -o - %s -funwind-tables-fno-asynchronous-unwind-tables | FileCheck %s -check-prefixes=CHECK,TABLES
+// RUN: %clang -target x86_64-linux -S -emit-llvm -o - %s -fno-unwind-tables -fno-asynchronous-unwind-tables | FileCheck %s -check-prefixes=CHECK,NO_TABLES
 
-// RUN: %clang -S -emit-llvm -o - -x c++ %s | FileCheck %s -check-prefixes=CHECK,DEFAULT
-// RUN: %clang -S -emit-llvm -o - -x c++ %s  -funwind-tables-fno-asynchronous-unwind-tables | FileCheck %s -check-prefixes=CHECK,TABLES
-// RUN: %clang -S -emit-llvm -o - -x c++ %s  -fno-exceptions -fno-unwind-tables -fno-asynchronous-unwind-tables | FileCheck %s -check-prefixes=CHECK,NO_TABLES
+// RUN: %clang -target x86_64-linux -S -emit-llvm -o - -x c++ %s | FileCheck %s -check-prefixes=CHECK,DEFAULT
+// RUN: %clang -target x86_64-linux -S -emit-llvm -o - -x c++ %s  -funwind-tables-fno-asynchronous-unwind-tables | FileCheck %s -check-prefixes=CHECK,TABLES
+// RUN: %clang -target x86_64-linux -S -emit-llvm -o - -x c++ %s 

[PATCH] D114543: Extend the `uwtable` attribute with unwind table kind

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D114543#3319347 , @durin42 wrote:

> As far as I can tell this patch broke the Rust compiler, but from the commit 
> message it sounds like it shouldn't have?
>
> https://buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds/8358#e85ad6f3-9992-4ea1-9cd3-d8db9f45f33e
>  fails with
>
>   Attribute 'uwtable' should have an Argument
>   i8* (i64, i64)* @__rust_alloc
>   in function __rust_alloc
>   LLVM ERROR: Broken function found, compilation aborted!
>
> Any thoughts?

Yeah, that's puzzling. The attribute has an optional argument (or else we had 
to update ~3080 occurrences  of "uwtable" in tests), so reading it is
a bit tricky:  
https://github.com/llvm/llvm-project/blob/19b4e9d76ecc9a5343c093bc54d965734b996518/llvm/lib/Bitcode/Reader/BitcodeReader.cpp#L1631
That message is output here 
https://github.com/llvm/llvm-project/blob/19b4e9d76ecc9a5343c093bc54d965734b996518/llvm/lib/IR/Verifier.cpp#L1710
and I can trigger this line with

  $ cat x.ll
  define void @f() uwtable {
ret void
  }
  $ ./bin/opt -S --passes=verify x.ll
  ; ModuleID = 'x.ll'
  source_filename = "x.ll"
  
  ; Function Attrs: uwtable
  define void @f() #0 {
ret void
  }
  
  attributes #0 = { uwtable }
  $ ./bin/opt  x.ll -o x.bc
  $ ./bin/opt --verify  x.bc -S
  ; ModuleID = 'x.bc'
  source_filename = "x.ll"
  
  ; Function Attrs: uwtable
  define void @f() #0 {
ret void
  }
  
  attributes #0 = { uwtable }
  $ 

Could there be a mismatch between two `llvm-project` versions, somehow?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114543

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

[PATCH] D114543: Extend the `uwtable` attribute with unwind table kind

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D114543#3319587 , @durin42 wrote:

> 



> Is the parameter optional if uwtable is set programmatically, or only when 
> we're reading IR streams?

No, it's not optional, the attribute is added by 
https://github.com/llvm/llvm-project/blob/00cd6c04202acf71f74c670b2dd4343929d1f45f/llvm/include/llvm/IR/Function.h#L636
(although seting it to `None` is semantically as not setting it at all).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114543

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

[PATCH] D114543: Extend the `uwtable` attribute with unwind table kind

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: llvm/lib/IR/Attributes.cpp:453
+return "uwtable";
+  return ("uwtable(" + Twine(Kind == UWTableKind::Sync ? "sync" : "async") 
+
+  ")")

RKSimon wrote:
> @chill Static analysis is warning that its impossible to hit the if(Kind == 
> Default) case here - it looks like you have merged 2 versions of the same 
> (Kind != UWTableKind::None) handling code?
Thanks, indeed.

Fixed in https://reviews.llvm.org/D120030


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114543

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

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.
Herald added a project: LLVM.

Shouldn't we disable `OPT_ffine_grained_bitfield_accesses` only if TSAN is 
active?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D36562



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

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D36562#1641930 , @wmi wrote:

> In D36562#1639441 , @chill wrote:
>
> > Shouldn't we disable `OPT_ffine_grained_bitfield_accesses` only if TSAN is 
> > active?
>
>
> I don't remember why it is disabled for all sanitizer modes. Seems you are 
> right that the disabling the option is only necessary for TSAN. Do you have 
> actual needs for the option to be functioning on other sanitizer modes?


Well, yes and no. We have the option enabled by default and it causes a warning 
when we use it together with `-fsanitize=memtag` (we aren't really concerned 
with other sanitizers). That warning broke a few builds (e.g. CMake doing tests 
and not wanting to see *any* diagnostics. We can work around that in a number 
of ways, e.g. we can leave the default off for AArch64.

I'd prefer though to have an upstream solution, if that's considered beneficial 
for all LLVM users and this one seems like such a case: let anyone use the 
option with sanitizers, unless it's known that some sanitizers'utility is 
affected negatively (as with TSAN).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D36562



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

[PATCH] D33676: Place implictly declared functions at block scope

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Ping.


https://reviews.llvm.org/D33676



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