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

[Carey Williams via Phabricator via cfe-commits]

carwil added a comment.

Reverted.


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

[Carey Williams via Phabricator via cfe-commits]

carwil added a comment.

Hi @efriedma, thanks your comments. You're right, that was hasty of me. 
Apologies for that, it won't happen again.

RE: R6 
Case in point: you're right. We're definitely not handling this correctly. I'll 
revert the patch, or ask @anwel too. Once we have a fix, we'll raise another 
review.


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

[Carey Williams via Phabricator via cfe-commits]

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

I think there's been plenty of time for comments here. LGTM.


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

[Carey Williams via Phabricator via cfe-commits]

carwil added a comment.

Just want to double check with @efriedma, before we except this. I believe this 
patch now catches all the points you made on https://reviews.llvm.org/D56005. 
Anything we've missed?


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

[Carey Williams via Phabricator via cfe-commits]

carwil added a comment.

In D68862#1708132 , @chill wrote:

> 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.


Ah, I see. That's a fair comment. AArch64 also achieves it's -reserve-xN 
options via subtarget features (despite function context), I think that's where 
the inspiration/suggestion for doing it this way in the ARM backend came from.


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

[Carey Williams via Phabricator via cfe-commits]

carwil added a comment.

> 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.


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] D56003: [RFC] [CFE] Allocatable Global Register Variables for ARM

[Carey Williams via Phabricator via cfe-commits]

carwil abandoned this revision.
carwil added a comment.

Superseded by https://reviews.llvm.org/D68862.


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

https://reviews.llvm.org/D56003



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

[PATCH] D65000: [ARM] Set default alignment to 64bits

[Carey Williams via Phabricator via cfe-commits]

carwil added a comment.

In D65000#1595920 , @peter.smith wrote:

> test case missing A8 aside this looks ok to me. Would like to see if there 
> are any comments from the Pacific time zone.


Are you happy to approve this now @peter.smith?


Repository:
  rC Clang

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

https://reviews.llvm.org/D65000



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

[PATCH] D59094: [ARM] Fix bug 39982 - pcs("aapcs-vfp") is not consistent

[Carey Williams via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356776: [ARM] Fix bug 39982 - pcs(aapcs-vfp) is 
not consistent (authored by carwil, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59094?vs=190798=191895#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D59094

Files:
  lib/CodeGen/TargetInfo.cpp
  test/CodeGenCXX/arm-pcs.cpp

Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -5597,8 +5597,10 @@
   ABIKind getABIKind() const { return Kind; }
 
 private:
-  ABIArgInfo classifyReturnType(QualType RetTy, bool isVariadic) const;
-  ABIArgInfo classifyArgumentType(QualType RetTy, bool isVariadic) const;
+  ABIArgInfo classifyReturnType(QualType RetTy, bool isVariadic,
+unsigned functionCallConv) const;
+  ABIArgInfo classifyArgumentType(QualType RetTy, bool isVariadic,
+  unsigned functionCallConv) const;
   ABIArgInfo classifyHomogeneousAggregate(QualType Ty, const Type *Base,
   uint64_t Members) const;
   ABIArgInfo coerceIllegalVector(QualType Ty) const;
@@ -5608,6 +5610,8 @@
   bool isHomogeneousAggregateSmallEnough(const Type *Ty,
  uint64_t Members) const override;
 
+  bool isEffectivelyAAPCS_VFP(unsigned callConvention, bool acceptHalf) const;
+
   void computeInfo(CGFunctionInfo ) const override;
 
   Address EmitVAArg(CodeGenFunction , Address VAListAddr,
@@ -5728,11 +5732,13 @@
 
 void ARMABIInfo::computeInfo(CGFunctionInfo ) const {
   if (!::classifyReturnType(getCXXABI(), FI, *this))
-FI.getReturnInfo() =
-classifyReturnType(FI.getReturnType(), FI.isVariadic());
+FI.getReturnInfo() = classifyReturnType(FI.getReturnType(), FI.isVariadic(),
+FI.getCallingConvention());
 
   for (auto  : FI.arguments())
-I.info = classifyArgumentType(I.type, FI.isVariadic());
+I.info = classifyArgumentType(I.type, FI.isVariadic(),
+  FI.getCallingConvention());
+
 
   // Always honor user-specified calling convention.
   if (FI.getCallingConvention() != llvm::CallingConv::C)
@@ -5811,8 +5817,8 @@
   return ABIArgInfo::getDirect(nullptr, 0, nullptr, false);
 }
 
-ABIArgInfo ARMABIInfo::classifyArgumentType(QualType Ty,
-bool isVariadic) const {
+ABIArgInfo ARMABIInfo::classifyArgumentType(QualType Ty, bool isVariadic,
+unsigned functionCallConv) const {
   // 6.1.2.1 The following argument types are VFP CPRCs:
   //   A single-precision floating-point type (including promoted
   //   half-precision types); A double-precision floating-point type;
@@ -5820,7 +5826,9 @@
   //   with a Base Type of a single- or double-precision floating-point type,
   //   64-bit containerized vectors or 128-bit containerized vectors with one
   //   to four Elements.
-  bool IsEffectivelyAAPCS_VFP = getABIKind() == AAPCS_VFP && !isVariadic;
+  // Variadic functions should always marshal to the base standard.
+  bool IsAAPCS_VFP =
+  !isVariadic && isEffectivelyAAPCS_VFP(functionCallConv, /* AAPCS16 */ false);
 
   Ty = useFirstFieldIfTransparentUnion(Ty);
 
@@ -5833,7 +5841,7 @@
   // half type natively, and does not need to interwork with AAPCS code.
   if ((Ty->isFloat16Type() || Ty->isHalfType()) &&
   !getContext().getLangOpts().NativeHalfArgsAndReturns) {
-llvm::Type *ResType = IsEffectivelyAAPCS_VFP ?
+llvm::Type *ResType = IsAAPCS_VFP ?
   llvm::Type::getFloatTy(getVMContext()) :
   llvm::Type::getInt32Ty(getVMContext());
 return ABIArgInfo::getDirect(ResType);
@@ -5857,7 +5865,7 @@
   if (isEmptyRecord(getContext(), Ty, true))
 return ABIArgInfo::getIgnore();
 
-  if (IsEffectivelyAAPCS_VFP) {
+  if (IsAAPCS_VFP) {
 // Homogeneous Aggregates need to be expanded when we can fit the aggregate
 // into VFP registers.
 const Type *Base = nullptr;
@@ -6014,10 +6022,12 @@
   return true;
 }
 
-ABIArgInfo ARMABIInfo::classifyReturnType(QualType RetTy,
-  bool isVariadic) const {
-  bool IsEffectivelyAAPCS_VFP =
-  (getABIKind() == AAPCS_VFP || getABIKind() == AAPCS16_VFP) && !isVariadic;
+ABIArgInfo ARMABIInfo::classifyReturnType(QualType RetTy, bool isVariadic,
+  unsigned functionCallConv) const {
+
+  // Variadic functions should always marshal to the base standard.
+  bool IsAAPCS_VFP =
+  !isVariadic && isEffectivelyAAPCS_VFP(functionCallConv, /* AAPCS16 */ true);
 
   if (RetTy->isVoidType())
 return ABIArgInfo::getIgnore();
@@ -6038,7 +6048,7 @@
   // half type natively, and does not need to interwork with AAPCS code.
  

[PATCH] D59094: [ARM] Fix bug 39982 - pcs("aapcs-vfp") is not consistent

[Carey Williams via Phabricator via cfe-commits]

carwil updated this revision to Diff 190798.
carwil added a comment.

Done. I've added an extra parameter over what you might expect as 
classifyArgumentTypes doesn't seem to consider AAPCS16, whereas 
classifyReturnTypes does.
I've also renamed the variable to make the distinction between it and the 
function clearer.


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

https://reviews.llvm.org/D59094

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGenCXX/arm-pcs.cpp

Index: clang/test/CodeGenCXX/arm-pcs.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/arm-pcs.cpp
@@ -0,0 +1,51 @@
+// Covers a bug fix for ABI selection with homogenous aggregates:
+//  See: https://bugs.llvm.org/show_bug.cgi?id=39982
+
+// REQUIRES: arm-registered-target
+// RUN: %clang -mfloat-abi=hard --target=armv7-unknown-linux-gnueabi -O3 -S -o - %s | FileCheck %s -check-prefixes=HARD,CHECK
+// RUN: %clang -mfloat-abi=softfp --target=armv7-unknown-linux-gnueabi -O3 -S -o - %s | FileCheck %s -check-prefixes=SOFTFP,CHECK
+// RUN: %clang -mfloat-abi=soft --target=armv7-unknown-linux-gnueabi -O3 -S -o - %s | FileCheck %s -check-prefixes=SOFT,CHECK
+
+struct S {
+  float f;
+  float d;
+  float c;
+  float t;
+};
+
+// Variadic functions should always marshal for the base standard.
+// See section 5.5 (Parameter Passing) of the AAPCS.
+float __attribute__((pcs("aapcs-vfp"))) variadic(S s, ...) {
+  // CHECK-NOT: vmov s{{[0-9]+}}, s{{[0-9]+}}
+  // CHECK: mov r{{[0-9]+}}, r{{[0-9]+}}
+  return s.d;
+}
+
+float no_attribute(S s) {
+  // SOFT: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // SOFTFP: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // HARD: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  return s.d;
+}
+
+float __attribute__((pcs("aapcs-vfp"))) baz(float x, float y) {
+  // CHECK-NOT: mov s{{[0-9]+}}, r{{[0-9]+}}
+  // SOFT: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // SOFTFP: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  // HARD: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  return y;
+}
+
+float __attribute__((pcs("aapcs-vfp"))) foo(S s) {
+  // CHECK-NOT: mov s{{[0-9]+}}, r{{[0-9]+}}
+  // SOFT: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // SOFTFP: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  // HARD: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  return s.d;
+}
+
+float __attribute__((pcs("aapcs"))) bar(S s) {
+  // CHECK-NOT: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  // CHECK: mov r{{[0-9]+}}, r{{[0-9]+}}
+  return s.d;
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -5591,8 +5591,10 @@
   ABIKind getABIKind() const { return Kind; }
 
 private:
-  ABIArgInfo classifyReturnType(QualType RetTy, bool isVariadic) const;
-  ABIArgInfo classifyArgumentType(QualType RetTy, bool isVariadic) const;
+  ABIArgInfo classifyReturnType(QualType RetTy, bool isVariadic,
+unsigned functionCallConv) const;
+  ABIArgInfo classifyArgumentType(QualType RetTy, bool isVariadic,
+  unsigned functionCallConv) const;
   ABIArgInfo classifyHomogeneousAggregate(QualType Ty, const Type *Base,
   uint64_t Members) const;
   ABIArgInfo coerceIllegalVector(QualType Ty) const;
@@ -5602,6 +5604,8 @@
   bool isHomogeneousAggregateSmallEnough(const Type *Ty,
  uint64_t Members) const override;
 
+  bool isEffectivelyAAPCS_VFP(unsigned callConvention, bool acceptHalf) const;
+
   void computeInfo(CGFunctionInfo ) const override;
 
   Address EmitVAArg(CodeGenFunction , Address VAListAddr,
@@ -5722,11 +5726,13 @@
 
 void ARMABIInfo::computeInfo(CGFunctionInfo ) const {
   if (!::classifyReturnType(getCXXABI(), FI, *this))
-FI.getReturnInfo() =
-classifyReturnType(FI.getReturnType(), FI.isVariadic());
+FI.getReturnInfo() = classifyReturnType(FI.getReturnType(), FI.isVariadic(),
+FI.getCallingConvention());
 
   for (auto  : FI.arguments())
-I.info = classifyArgumentType(I.type, FI.isVariadic());
+I.info = classifyArgumentType(I.type, FI.isVariadic(),
+  FI.getCallingConvention());
+
 
   // Always honor user-specified calling convention.
   if (FI.getCallingConvention() != llvm::CallingConv::C)
@@ -5805,8 +5811,8 @@
   return ABIArgInfo::getDirect(nullptr, 0, nullptr, false);
 }
 
-ABIArgInfo ARMABIInfo::classifyArgumentType(QualType Ty,
-bool isVariadic) const {
+ABIArgInfo ARMABIInfo::classifyArgumentType(QualType Ty, bool isVariadic,
+unsigned functionCallConv) const {
   // 6.1.2.1 The following argument types are VFP CPRCs:
   //   A single-precision floating-point type (including promoted
   //   half-precision types); A double-precision floating-point type;
@@ -5814,7 +5820,9 @@
   //   with a Base Type 

[PATCH] D59094: [ARM] Fix bug 39982 - pcs("aapcs-vfp") is not consistent

[Carey Williams via Phabricator via cfe-commits]

carwil updated this revision to Diff 190653.
carwil marked an inline comment as done.
carwil added a comment.

I got bit a confused earlier. That does actually seem like correct behaviour 
(once we're no longer able to treat the struct as a homogeneous aggregate).
I've tightened up the conditional a bit as it wouldn't have previously 
accounted for the case where you had mfloat-abi=hard with an AAPCS (non VFP) 
attribute. Although in this instance, the backend does seem to be 
correcting/ignoring that, it's still best we don't blindly rely on such 
behaviour.

I've also re-structured the conditional and added some comments so it's 
(hopefully) a little bit clearer what's going on.

I believe this fixes the bug at hand. There's some other calls to getABIKind 
referencing AAPCS16_VFP, but that doesn't have a direct translation to an 
LLVM/FI CallingConvention (that I can see). Seems there is a lot of different 
ways to check various ABI/CC's and none of them seem able to give you the full 
picture at any point.


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

https://reviews.llvm.org/D59094

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGenCXX/arm-pcs.cpp

Index: clang/test/CodeGenCXX/arm-pcs.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/arm-pcs.cpp
@@ -0,0 +1,51 @@
+// Covers a bug fix for ABI selection with homogenous aggregates:
+//  See: https://bugs.llvm.org/show_bug.cgi?id=39982
+
+// REQUIRES: arm-registered-target
+// RUN: %clang -mfloat-abi=hard --target=armv7-unknown-linux-gnueabi -O3 -S -o - %s | FileCheck %s -check-prefixes=HARD,CHECK
+// RUN: %clang -mfloat-abi=softfp --target=armv7-unknown-linux-gnueabi -O3 -S -o - %s | FileCheck %s -check-prefixes=SOFTFP,CHECK
+// RUN: %clang -mfloat-abi=soft --target=armv7-unknown-linux-gnueabi -O3 -S -o - %s | FileCheck %s -check-prefixes=SOFT,CHECK
+
+struct S {
+  float f;
+  float d;
+  float c;
+  float t;
+};
+
+// Variadic functions should always marshal for the base standard.
+// See section 5.5 (Parameter Passing) of the AAPCS.
+float __attribute__((pcs("aapcs-vfp"))) variadic(S s, ...) {
+  // CHECK-NOT: vmov s{{[0-9]+}}, s{{[0-9]+}}
+  // CHECK: mov r{{[0-9]+}}, r{{[0-9]+}}
+  return s.d;
+}
+
+float no_attribute(S s) {
+  // SOFT: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // SOFTFP: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // HARD: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  return s.d;
+}
+
+float __attribute__((pcs("aapcs-vfp"))) baz(float x, float y) {
+  // CHECK-NOT: mov s{{[0-9]+}}, r{{[0-9]+}}
+  // SOFT: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // SOFTFP: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  // HARD: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  return y;
+}
+
+float __attribute__((pcs("aapcs-vfp"))) foo(S s) {
+  // CHECK-NOT: mov s{{[0-9]+}}, r{{[0-9]+}}
+  // SOFT: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // SOFTFP: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  // HARD: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  return s.d;
+}
+
+float __attribute__((pcs("aapcs"))) bar(S s) {
+  // CHECK-NOT: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  // CHECK: mov r{{[0-9]+}}, r{{[0-9]+}}
+  return s.d;
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -10,7 +10,6 @@
 // definition used to handle ABI compliancy.
 //
 //===--===//
-
 #include "TargetInfo.h"
 #include "ABIInfo.h"
 #include "CGBlocks.h"
@@ -5591,8 +5590,10 @@
   ABIKind getABIKind() const { return Kind; }
 
 private:
-  ABIArgInfo classifyReturnType(QualType RetTy, bool isVariadic) const;
-  ABIArgInfo classifyArgumentType(QualType RetTy, bool isVariadic) const;
+  ABIArgInfo classifyReturnType(QualType RetTy, bool isVariadic,
+unsigned functionCallConv) const;
+  ABIArgInfo classifyArgumentType(QualType RetTy, bool isVariadic,
+  unsigned functionCallConv) const;
   ABIArgInfo classifyHomogeneousAggregate(QualType Ty, const Type *Base,
   uint64_t Members) const;
   ABIArgInfo coerceIllegalVector(QualType Ty) const;
@@ -5722,11 +5723,13 @@
 
 void ARMABIInfo::computeInfo(CGFunctionInfo ) const {
   if (!::classifyReturnType(getCXXABI(), FI, *this))
-FI.getReturnInfo() =
-classifyReturnType(FI.getReturnType(), FI.isVariadic());
+FI.getReturnInfo() = classifyReturnType(FI.getReturnType(), FI.isVariadic(),
+FI.getCallingConvention());
 
   for (auto  : FI.arguments())
-I.info = classifyArgumentType(I.type, FI.isVariadic());
+I.info = classifyArgumentType(I.type, FI.isVariadic(),
+  FI.getCallingConvention());
+
 
   // Always honor user-specified calling convention.
   if (FI.getCallingConvention() != llvm::CallingConv::C)
@@ -5805,8 +5808,8 @@
   return 

[PATCH] D59094: [ARM] Fix bug 39982 - pcs("aapcs-vfp") is not consistent

[Carey Williams via Phabricator via cfe-commits]

carwil added a comment.

In D59094#1421893 , @efriedma wrote:

> Not sure how to write a testcase off the top of my head... have you tried 
> homogeneous aggregates with more than two elements?


Good catch! Seems like if we surpass "isHomogeneousAggregateSmallEnough" (4), 
it's no longer classfied as an aggregate and so doesn't benefit from this fix. 
I'll see if I can work out a way to catch that as well.

> It looks like every call to getABIKind() is doing the wrong thing; I'm 
> worried that's going to lead to some sort of subtle miscompile without some 
> sort of centralized fix to compute the correct calling convention for every 
> place that checks it.

Agreed. It would be much better if this determined the ABI on context. That 
said, it doesn't look like that's going to be trivial to achieve...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59094



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

[PATCH] D59094: [ARM] Fix bug 39982 - pcs("aapcs-vfp") is not consistent

[Carey Williams via Phabricator via cfe-commits]

carwil created this revision.
Herald added subscribers: cfe-commits, jdoerfert, kristof.beyls, javed.absar.
Herald added a project: clang.

See: https://bugs.llvm.org/show_bug.cgi?id=39982

When considering how to classify homogeneous aggregates as return/argument 
types, the ABI of the function (specified by attribute pcs) wasn't being taken 
into account. This resulted in some weird and unexpected hybrid assembly when 
compiling with softfp.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59094

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGenCXX/arm-pcs.cpp

Index: clang/test/CodeGenCXX/arm-pcs.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/arm-pcs.cpp
@@ -0,0 +1,46 @@
+// Covers a bug fix for ABI selection with homogenous aggregates:
+//  See: https://bugs.llvm.org/show_bug.cgi?id=39982
+
+// RUN: %clang -mfloat-abi=hard --target=armv7-unknown-linux-gnueabi -O3 -S -o - %s | FileCheck %s -check-prefixes=HARD_INSTRs,CHECK
+// RUN: %clang -mfloat-abi=softfp --target=armv7-unknown-linux-gnueabi -O3 -S -o - %s | FileCheck %s -check-prefixes=HARD_INSTRs,CHECK
+// RUN: %clang -mfloat-abi=soft --target=armv7-unknown-linux-gnueabi -O3 -S -o - %s | FileCheck %s -check-prefixes=SOFT_INSTRs,CHECK
+
+struct S {
+  float f;
+  float d;
+};
+
+// Variadic functions should always marshal for the base standard.
+// See section 5.5 (Parameter Passing) of the AAPCS.
+float __attribute__((pcs("aapcs-vfp"))) variadic(S s, ...) {
+  // CHECK-NOT: vmov s{{[0-9]+}}, s{{[0-9]+}}
+  // CHECK: mov r{{[0-9]+}}, r{{[0-9]+}}
+  return s.d;
+}
+
+float __attribute__((pcs("aapcs-vfp"))) not_variadic(S s) {
+  // SOFT_INSTRs: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // HARD_INSTRs-NOT: vmov s{{[0-9]+}}, r{{[0-9]+}}
+  // HARD_INSTRs: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  return s.d;
+}
+
+float __attribute__((pcs("aapcs-vfp"))) baz(float x, float y) {
+  // SOFT_INSTRs: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // HARD_INSTRs-NOT: mov s{{[0-9]+}}, r{{[0-9]+}}
+  // HARD_INSTRs: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  return y;
+}
+
+float __attribute__((pcs("aapcs-vfp"))) foo(S s) {
+  // SOFT_INSTRs: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // HARD_INSTRs-NOT: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // HARD_INSTRs: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  return s.d;
+}
+
+float __attribute__((pcs("aapcs"))) bar(S s) {
+  // CHECK-NOT: vmov s{{[0-9]+}}, s{{[0-9]+}}
+  // CHECK: mov r{{[0-9]+}}, r{{[0-9]+}}
+  return s.d;
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -5591,8 +5591,10 @@
   ABIKind getABIKind() const { return Kind; }
 
 private:
-  ABIArgInfo classifyReturnType(QualType RetTy, bool isVariadic) const;
-  ABIArgInfo classifyArgumentType(QualType RetTy, bool isVariadic) const;
+  ABIArgInfo classifyReturnType(QualType RetTy, bool isVariadic,
+unsigned functionCallConv) const;
+  ABIArgInfo classifyArgumentType(QualType RetTy, bool isVariadic,
+  unsigned functionCallConv) const;
   ABIArgInfo classifyHomogeneousAggregate(QualType Ty, const Type *Base,
   uint64_t Members) const;
   ABIArgInfo coerceIllegalVector(QualType Ty) const;
@@ -5722,11 +5724,12 @@
 
 void ARMABIInfo::computeInfo(CGFunctionInfo ) const {
   if (!::classifyReturnType(getCXXABI(), FI, *this))
-FI.getReturnInfo() =
-classifyReturnType(FI.getReturnType(), FI.isVariadic());
+FI.getReturnInfo() = classifyReturnType(FI.getReturnType(), FI.isVariadic(),
+FI.getCallingConvention());
 
   for (auto  : FI.arguments())
-I.info = classifyArgumentType(I.type, FI.isVariadic());
+I.info = classifyArgumentType(I.type, FI.isVariadic(),
+  FI.getCallingConvention());
 
   // Always honor user-specified calling convention.
   if (FI.getCallingConvention() != llvm::CallingConv::C)
@@ -5805,8 +5808,8 @@
   return ABIArgInfo::getDirect(nullptr, 0, nullptr, false);
 }
 
-ABIArgInfo ARMABIInfo::classifyArgumentType(QualType Ty,
-bool isVariadic) const {
+ABIArgInfo ARMABIInfo::classifyArgumentType(QualType Ty, bool isVariadic,
+unsigned functionCallConv) const {
   // 6.1.2.1 The following argument types are VFP CPRCs:
   //   A single-precision floating-point type (including promoted
   //   half-precision types); A double-precision floating-point type;
@@ -5814,8 +5817,11 @@
   //   with a Base Type of a single- or double-precision floating-point type,
   //   64-bit containerized vectors or 128-bit containerized vectors with one
   //   to four Elements.
-  bool IsEffectivelyAAPCS_VFP = getABIKind() == AAPCS_VFP && !isVariadic;
 
+  bool IsEffectivelyAAPCS_VFP =
+  (getABIKind() == 

[PATCH] D56003: [RFC] [CFE] Allocatable Global Register Variables for ARM

[Carey Williams via Phabricator via cfe-commits]

carwil updated this revision to Diff 185277.
carwil added a comment.

Removed the complicated frame pointer/ffixed combination errors in favour of an 
always on warning (in a new group, so it can be silenced).


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

https://reviews.llvm.org/D56003

Files:
  docs/ClangCommandLineReference.rst
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Driver/Options.td
  lib/Basic/Targets/ARM.cpp
  lib/Basic/Targets/ARM.h
  lib/Driver/ToolChains/Arch/ARM.cpp
  test/Driver/arm-reserved-reg-options.c
  test/Sema/arm-global-regs.c

Index: test/Sema/arm-global-regs.c
===
--- /dev/null
+++ test/Sema/arm-global-regs.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -ffreestanding -fsyntax-only -verify -triple arm %s
+
+// Check a small subset of valid and invalid global register variable declarations.
+
+register unsigned arm_r3 __asm("r3");   //expected-error {{register 'r3' unsuitable for global register variables on this target}}
+
+register unsigned arm_r12 __asm("r12"); //expected-error {{register 'r12' unsuitable for global register variables on this target}}
+
+register unsigned arm_r5 __asm("r5");
+
+register unsigned arm_r9 __asm("r9");
+
+register unsigned arm_sp __asm("sp");
\ No newline at end of file
Index: test/Driver/arm-reserved-reg-options.c
===
--- /dev/null
+++ test/Driver/arm-reserved-reg-options.c
@@ -0,0 +1,35 @@
+// ## FP ARM + Thumb
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -c %s 2>&1 | FileCheck -check-prefix=CHECK-WARNING-R11 %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -Wno-fixed-registers -c %s 2>&1 | FileCheck -check-prefix=CHECK-NO-WARNING %s
+
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -mthumb -c %s 2>&1 | FileCheck -check-prefix=CHECK-WARNING-R7 %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -mthumb -Wno-fixed-registers -c %s 2>&1 | FileCheck -check-prefix=CHECK-NO-WARNING %s
+
+// RUN: %clang -target thumbv6m-none-eabi -### -ffixed-r7 -c %s 2>&1 | FileCheck -check-prefix=CHECK-WARNING-R7 %s
+// RUN: %clang -target thumbv6m-none-eabi -### -ffixed-r7 -Wno-fixed-registers -c %s 2>&1 | FileCheck -check-prefix=CHECK-NO-WARNING %s
+
+// ## FP Darwin (R7)
+// RUN: %clang -target armv6-apple-darwin9 -### -ffixed-r7 -c %s 2>&1 | FileCheck -check-prefix=CHECK-WARNING-R7 %s
+// RUN: %clang -target armv6-apple-darwin9 -### -ffixed-r7 -Wno-fixed-registers -c %s 2>&1 | FileCheck -check-prefix=CHECK-NO-WARNING %s
+
+// RUN: %clang -target armv6-apple-ios3 -### -ffixed-r7 -c %s 2>&1 | FileCheck -check-prefix=CHECK-WARNING-R7 %s
+// RUN: %clang -target armv6-apple-ios3 -### -ffixed-r7 -Wno-fixed-registers -c %s 2>&1 | FileCheck -check-prefix=CHECK-NO-WARNING %s
+
+// RUN: %clang -target armv7s-apple-darwin10 -### -ffixed-r7 -c %s 2>&1 | FileCheck -check-prefix=CHECK-WARNING-R7 %s
+// RUN: %clang -target armv7s-apple-darwin10 -### -ffixed-r7 -Wno-fixed-registers -c %s 2>&1 | FileCheck -check-prefix=CHECK-NO-WARNING %s
+
+// ## FP Windows (R11)
+// RUN: %clang -target armv7-windows -### -ffixed-r11 -c %s 2>&1 | FileCheck -check-prefix=CHECK-WARNING-R11 %s
+// RUN: %clang -target armv7-windows -### -ffixed-r11 -Wno-fixed-registers -c %s 2>&1 | FileCheck -check-prefix=CHECK-NO-WARNING %s
+
+// ## FRWPI (R9)
+// RUN: %clang -target arm-arm-none-eabi -### -frwpi -ffixed-r9 -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-FRWPI-CONFLICT %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r9 -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-FRWPI-VALID %s
+// RUN: %clang -target arm-arm-none-eabi -### -frwpi -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-FRWPI-VALID %s
+
+// CHECK-WARNING-R11: warning: '-ffixed-r11' has been specified but 'r11' may still be used as a frame pointer [-Wfixed-registers]
+// CHECK-WARNING-R7: warning: '-ffixed-r7' has been specified but 'r7' may still be used as a frame pointer [-Wfixed-registers]
+// CHECK-NO-WARNING-NOT: may still be used as a frame pointer [-Wfixed-registers]
+
+// CHECK-RESERVED-FRWPI-CONFLICT: option '-ffixed-r9' cannot be specified with '-frwpi'
+// CHECK-RESERVED-FRWPI-VALID-NOT: option '-ffixed-r9' cannot be specified with '-frwpi'
\ No newline at end of file
Index: lib/Driver/ToolChains/Arch/ARM.cpp
===
--- lib/Driver/ToolChains/Arch/ARM.cpp
+++ lib/Driver/ToolChains/Arch/ARM.cpp
@@ -547,11 +547,37 @@
   Features.push_back("+strict-align");
   }
 
-  // llvm does not support reserving registers in general. There is support
-  // for reserving r9 on ARM though (defined as a platform-specific register
-  // in ARM EABI).
-  if (Args.hasArg(options::OPT_ffixed_r9))
-Features.push_back("+reserve-r9");
+  // Do not allow r9 reservation with -frwpi.
+  if 

[PATCH] D56003: [RFC] [CFE] Allocatable Global Register Variables for ARM

[Carey Williams via Phabricator via cfe-commits]

carwil updated this revision to Diff 183114.
carwil added a comment.

More tests, and better handling of argument combination errors.


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

https://reviews.llvm.org/D56003

Files:
  docs/ClangCommandLineReference.rst
  include/clang/Driver/Options.td
  lib/Basic/Targets/ARM.cpp
  lib/Basic/Targets/ARM.h
  lib/Driver/ToolChains/Arch/ARM.cpp
  test/Driver/arm-reserved-reg-options.c
  test/Sema/arm-global-regs.c

Index: test/Sema/arm-global-regs.c
===
--- /dev/null
+++ test/Sema/arm-global-regs.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -ffreestanding -fsyntax-only -verify -triple arm %s
+
+// Check a small subset of valid and invalid global register variable declarations.
+
+register unsigned arm_r3 __asm("r3");   //expected-error {{register 'r3' unsuitable for global register variables on this target}}
+
+register unsigned arm_r12 __asm("r12"); //expected-error {{register 'r12' unsuitable for global register variables on this target}}
+
+register unsigned arm_r5 __asm("r5");
+
+register unsigned arm_r9 __asm("r9");
+
+register unsigned arm_sp __asm("sp");
\ No newline at end of file
Index: test/Driver/arm-reserved-reg-options.c
===
--- /dev/null
+++ test/Driver/arm-reserved-reg-options.c
@@ -0,0 +1,68 @@
+// ## FP ARM + Thumb
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R11-WITHOUT-OMIT %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -fno-omit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R11-WITH-NO-OMIT %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -fno-omit-frame-pointer -fomit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -fomit-frame-pointer -fno-omit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R11-WITH-NO-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -fomit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -mthumb -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITHOUT-OMIT %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -mthumb -fno-omit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-NO-OMIT %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -mthumb -fno-omit-frame-pointer -fomit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -mthumb -fomit-frame-pointer -fno-omit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-NO-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -mthumb -fomit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST %s
+
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -fno-omit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -fomit-frame-pointer -fno-omit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -fno-omit-frame-pointer -fomit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -mthumb -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -mthumb -fno-omit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -mthumb -fomit-frame-pointer -fno-omit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -mthumb -fno-omit-frame-pointer -fomit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+
+// RUN: %clang -target thumbv6m-none-eabi -### -ffixed-r7 -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITHOUT-OMIT %s
+// RUN: %clang -target thumbv6m-none-eabi -### -ffixed-r7 -fno-omit-frame-pointer -c %s 2>&1 | FileCheck 

[PATCH] D56003: [RFC] [CFE] Allocatable Global Register Variables for ARM

[Carey Williams via Phabricator via cfe-commits]

carwil updated this revision to Diff 181766.
carwil set the repository for this revision to rC Clang.
carwil added a comment.
Herald added a subscriber: cfe-commits.

Added cfe-commits.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56003

Files:
  docs/ClangCommandLineReference.rst
  include/clang/Driver/Options.td
  lib/Basic/Targets/ARM.cpp
  lib/Basic/Targets/ARM.h
  lib/Driver/ToolChains/Arch/ARM.cpp
  test/Driver/arm-reserved-regs.c
  test/Sema/arm-global-regs.c

Index: test/Sema/arm-global-regs.c
===
--- /dev/null
+++ test/Sema/arm-global-regs.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -ffreestanding -fsyntax-only -verify -triple arm %s
+
+// Check a small subset of valid and invalid global register variable declarations.
+
+register unsigned arm_r3 __asm("r3");   //expected-error {{register 'r3' unsuitable for global register variables on this target}}
+
+register unsigned arm_r12 __asm("r12"); //expected-error {{register 'r12' unsuitable for global register variables on this target}}
+
+register unsigned arm_r5 __asm("r5");
+
+register unsigned arm_r9 __asm("r9");
+
+register unsigned arm_sp __asm("sp");
\ No newline at end of file
Index: test/Driver/arm-reserved-regs.c
===
--- /dev/null
+++ test/Driver/arm-reserved-regs.c
@@ -0,0 +1,11 @@
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -c %s 2>&1 | FileCheck -check-prefix=CHECK-NO-RESERVED-FRAME-POINTER %s
+// CHECK-NO-RESERVED-FRAME-POINTER: option '-ffixed-r11' cannot be specified without '-fomit-frame-pointer'
+
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -mthumb -c %s 2>&1 | FileCheck -check-prefix=CHECK-NO-RESERVED-FRAME-POINTER-THUMB %s
+// CHECK-NO-RESERVED-FRAME-POINTER-THUMB: option '-ffixed-r7' cannot be specified without '-fomit-frame-pointer'
+
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -fomit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-NO-FRAME-POINTER %s
+// CHECK-RESERVED-NO-FRAME-POINTER-NOT: option '-ffixed-r11' cannot be specified without '-fomit-frame-pointer'
+
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -mthumb -fomit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-NO-FRAME-POINTER-THUMB %s
+// CHECK-RESERVED-NO-FRAME-POINTER-THUMB-NOT: option '-ffixed-r7' cannot be specified without '-fomit-frame-pointer'
\ No newline at end of file
Index: lib/Driver/ToolChains/Arch/ARM.cpp
===
--- lib/Driver/ToolChains/Arch/ARM.cpp
+++ lib/Driver/ToolChains/Arch/ARM.cpp
@@ -548,11 +548,42 @@
   Features.push_back("+strict-align");
   }
 
-  // llvm does not support reserving registers in general. There is support
-  // for reserving r9 on ARM though (defined as a platform-specific register
-  // in ARM EABI).
-  if (Args.hasArg(options::OPT_ffixed_r9))
-Features.push_back("+reserve-r9");
+  // Do not allow r9 reservation with -frwpi.
+  if (Args.hasArg(options::OPT_ffixed_r9) && Args.hasArg(options::OPT_frwpi)) {
+Arg *A = Args.getLastArg(options::OPT_ffixed_r9);
+Arg *B = Args.getLastArg(options::OPT_frwpi);
+D.Diag(diag::err_opt_not_valid_with_opt) << A->getAsString(Args) << B->getAsString(Args);
+  }
+
+  // Do not allow fp reservation unless it's been explicity omitted.
+  if (!Args.hasArg(options::OPT_fomit_frame_pointer) ||
+   Args.hasArg(options::OPT_fno_omit_frame_pointer)) {
+if (Triple.isOSDarwin() || (!Triple.isOSWindows() && Triple.isThumb())) {
+  if (Args.hasArg(options::OPT_ffixed_r7)) {
+Arg *A = Args.getLastArg(options::OPT_ffixed_r7);
+D.Diag(diag::err_opt_not_valid_without_opt) << A->getAsString(Args)
+<< "-fomit-frame-pointer"
+<< " or with -fno-omit-frame-pointer.";
+  }
+} else if (Args.hasArg(options::OPT_ffixed_r11)) {
+Arg *A = Args.getLastArg(options::OPT_ffixed_r11);
+D.Diag(diag::err_opt_not_valid_without_opt) << A->getAsString(Args)
+<< "-fomit-frame-pointer"
+<< " or with -fno-omit-frame-pointer.";
+}
+  }
+
+// Reservation of general purpose registers.
+#define HANDLE_FFIXED_R(n) \
+  if (Args.hasArg(options::OPT_ffixed_r##n)) \
+Features.push_back("+reserve-r" #n)
+  HANDLE_FFIXED_R(5);
+  HANDLE_FFIXED_R(6);
+  HANDLE_FFIXED_R(7);
+  HANDLE_FFIXED_R(8);
+  HANDLE_FFIXED_R(9);
+  HANDLE_FFIXED_R(10);
+  HANDLE_FFIXED_R(11);
 
   // The kext linker doesn't know how to deal with movw/movt.
   if (KernelOrKext || Args.hasArg(options::OPT_mno_movt))
Index: lib/Basic/Targets/ARM.h
===
--- lib/Basic/Targets/ARM.h
+++ 

[PATCH] D55428: [Docs] Expand -fstack-protector and -fstack-protector-all info

[Carey Williams via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL349335: [Docs] Expand -fstack-protector and 
-fstack-protector-all (authored by carwil, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55428?vs=177828=178434#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55428

Files:
  cfe/trunk/include/clang/Driver/Options.td


Index: cfe/trunk/include/clang/Driver/Options.td
===
--- cfe/trunk/include/clang/Driver/Options.td
+++ cfe/trunk/include/clang/Driver/Options.td
@@ -1639,11 +1639,18 @@
 Flags<[CC1Option]>, HelpText<"Char is unsigned">;
 def fsplit_stack : Flag<["-"], "fsplit-stack">, Group;
 def fstack_protector_all : Flag<["-"], "fstack-protector-all">, Group,
-  HelpText<"Force the usage of stack protectors for all functions">;
+  HelpText<"Enable stack protectors for all functions">;
 def fstack_protector_strong : Flag<["-"], "fstack-protector-strong">, 
Group,
-  HelpText<"Use a strong heuristic to apply stack protectors to functions">;
+  HelpText<"Enable stack protectors for some functions vulnerable to stack 
smashing. "
+   "Compared to -fstack-protector, this uses a stronger heuristic "
+   "that includes functions containing arrays of any size (and any 
type), "
+   "as well as any calls to alloca or the taking of an address from a 
local variable">;
 def fstack_protector : Flag<["-"], "fstack-protector">, Group,
-  HelpText<"Enable stack protectors for functions potentially vulnerable to 
stack smashing">;
+  HelpText<"Enable stack protectors for some functions vulnerable to stack 
smashing. "
+   "This uses a loose heuristic which considers functions vulnerable "
+   "if they contain a char (or 8bit integer) array or constant sized 
calls to "
+   "alloca, which are of greater size than ssp-buffer-size (default: 8 
bytes). "
+   "All variable sized calls to alloca are considered vulnerable">;
 def fstandalone_debug : Flag<["-"], "fstandalone-debug">, Group, 
Flags<[CoreOption]>,
   HelpText<"Emit full debug info for all types used by the program">;
 def fno_standalone_debug : Flag<["-"], "fno-standalone-debug">, 
Group, Flags<[CoreOption]>,


Index: cfe/trunk/include/clang/Driver/Options.td
===
--- cfe/trunk/include/clang/Driver/Options.td
+++ cfe/trunk/include/clang/Driver/Options.td
@@ -1639,11 +1639,18 @@
 Flags<[CC1Option]>, HelpText<"Char is unsigned">;
 def fsplit_stack : Flag<["-"], "fsplit-stack">, Group;
 def fstack_protector_all : Flag<["-"], "fstack-protector-all">, Group,
-  HelpText<"Force the usage of stack protectors for all functions">;
+  HelpText<"Enable stack protectors for all functions">;
 def fstack_protector_strong : Flag<["-"], "fstack-protector-strong">, Group,
-  HelpText<"Use a strong heuristic to apply stack protectors to functions">;
+  HelpText<"Enable stack protectors for some functions vulnerable to stack smashing. "
+   "Compared to -fstack-protector, this uses a stronger heuristic "
+   "that includes functions containing arrays of any size (and any type), "
+   "as well as any calls to alloca or the taking of an address from a local variable">;
 def fstack_protector : Flag<["-"], "fstack-protector">, Group,
-  HelpText<"Enable stack protectors for functions potentially vulnerable to stack smashing">;
+  HelpText<"Enable stack protectors for some functions vulnerable to stack smashing. "
+   "This uses a loose heuristic which considers functions vulnerable "
+   "if they contain a char (or 8bit integer) array or constant sized calls to "
+   "alloca, which are of greater size than ssp-buffer-size (default: 8 bytes). "
+   "All variable sized calls to alloca are considered vulnerable">;
 def fstandalone_debug : Flag<["-"], "fstandalone-debug">, Group, Flags<[CoreOption]>,
   HelpText<"Emit full debug info for all types used by the program">;
 def fno_standalone_debug : Flag<["-"], "fno-standalone-debug">, Group, Flags<[CoreOption]>,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55428: [Docs] Expand -fstack-protector and -fstack-protector-all info

[Carey Williams via Phabricator via cfe-commits]

carwil updated this revision to Diff 177828.
carwil marked an inline comment as done.
carwil added a comment.

Reworded -fstack-protector-all to bring it in line with the changes to the 
other two options.


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

https://reviews.llvm.org/D55428

Files:
  include/clang/Driver/Options.td


Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1633,11 +1633,18 @@
 Flags<[CC1Option]>, HelpText<"Char is unsigned">;
 def fsplit_stack : Flag<["-"], "fsplit-stack">, Group;
 def fstack_protector_all : Flag<["-"], "fstack-protector-all">, Group,
-  HelpText<"Force the usage of stack protectors for all functions">;
+  HelpText<"Enable stack protectors for all functions">;
 def fstack_protector_strong : Flag<["-"], "fstack-protector-strong">, 
Group,
-  HelpText<"Use a strong heuristic to apply stack protectors to functions">;
+  HelpText<"Enable stack protectors for some functions vulnerable to stack 
smashing. "
+   "Compared to -fstack-protector, this uses a stronger heuristic "
+   "that includes functions containing arrays of any size (and any 
type), "
+   "as well as any calls to alloca or the taking of an address from a 
local variable">;
 def fstack_protector : Flag<["-"], "fstack-protector">, Group,
-  HelpText<"Enable stack protectors for functions potentially vulnerable to 
stack smashing">;
+  HelpText<"Enable stack protectors for some functions vulnerable to stack 
smashing. "
+   "This uses a loose heuristic which considers functions vulnerable "
+   "if they contain a char (or 8bit integer) array or constant sized 
calls to "
+   "alloca, which are of greater size than ssp-buffer-size (default: 8 
bytes). "
+   "All variable sized calls to alloca are considered vulnerable">;
 def fstandalone_debug : Flag<["-"], "fstandalone-debug">, Group, 
Flags<[CoreOption]>,
   HelpText<"Emit full debug info for all types used by the program">;
 def fno_standalone_debug : Flag<["-"], "fno-standalone-debug">, 
Group, Flags<[CoreOption]>,


Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1633,11 +1633,18 @@
 Flags<[CC1Option]>, HelpText<"Char is unsigned">;
 def fsplit_stack : Flag<["-"], "fsplit-stack">, Group;
 def fstack_protector_all : Flag<["-"], "fstack-protector-all">, Group,
-  HelpText<"Force the usage of stack protectors for all functions">;
+  HelpText<"Enable stack protectors for all functions">;
 def fstack_protector_strong : Flag<["-"], "fstack-protector-strong">, Group,
-  HelpText<"Use a strong heuristic to apply stack protectors to functions">;
+  HelpText<"Enable stack protectors for some functions vulnerable to stack smashing. "
+   "Compared to -fstack-protector, this uses a stronger heuristic "
+   "that includes functions containing arrays of any size (and any type), "
+   "as well as any calls to alloca or the taking of an address from a local variable">;
 def fstack_protector : Flag<["-"], "fstack-protector">, Group,
-  HelpText<"Enable stack protectors for functions potentially vulnerable to stack smashing">;
+  HelpText<"Enable stack protectors for some functions vulnerable to stack smashing. "
+   "This uses a loose heuristic which considers functions vulnerable "
+   "if they contain a char (or 8bit integer) array or constant sized calls to "
+   "alloca, which are of greater size than ssp-buffer-size (default: 8 bytes). "
+   "All variable sized calls to alloca are considered vulnerable">;
 def fstandalone_debug : Flag<["-"], "fstandalone-debug">, Group, Flags<[CoreOption]>,
   HelpText<"Emit full debug info for all types used by the program">;
 def fno_standalone_debug : Flag<["-"], "fno-standalone-debug">, Group, Flags<[CoreOption]>,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55428: [Docs] Expand -fstack-protector and -fstack-protector-all info

[Carey Williams via Phabricator via cfe-commits]

carwil updated this revision to Diff 177668.
carwil added a comment.

Make thopre's suggested changes.


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

https://reviews.llvm.org/D55428

Files:
  include/clang/Driver/Options.td


Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1635,9 +1635,16 @@
 def fstack_protector_all : Flag<["-"], "fstack-protector-all">, Group,
   HelpText<"Force the usage of stack protectors for all functions">;
 def fstack_protector_strong : Flag<["-"], "fstack-protector-strong">, 
Group,
-  HelpText<"Use a strong heuristic to apply stack protectors to functions">;
+  HelpText<"Enable stack protectors for some functions vulnerable to stack 
smashing. "
+   "Compared to -fstack-protector, this uses a stronger heuristic "
+   "that includes functions containing arrays of any size (and any 
type), "
+   "as well as any calls to alloca or the taking of an address from a 
local variable">;
 def fstack_protector : Flag<["-"], "fstack-protector">, Group,
-  HelpText<"Enable stack protectors for functions potentially vulnerable to 
stack smashing">;
+  HelpText<"Enable stack protectors for some functions vulnerable to stack 
smashing. "
+   "This uses a loose heuristic, which considers functions vulnerable "
+   "if they contain a char (or 8bit integer) array or constant sized 
calls to "
+   "alloca, which are of greater size than ssp-buffer-size (default: 8 
bytes). "
+   "All variable sized calls to alloca are considered vulnerable">;
 def fstandalone_debug : Flag<["-"], "fstandalone-debug">, Group, 
Flags<[CoreOption]>,
   HelpText<"Emit full debug info for all types used by the program">;
 def fno_standalone_debug : Flag<["-"], "fno-standalone-debug">, 
Group, Flags<[CoreOption]>,


Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1635,9 +1635,16 @@
 def fstack_protector_all : Flag<["-"], "fstack-protector-all">, Group,
   HelpText<"Force the usage of stack protectors for all functions">;
 def fstack_protector_strong : Flag<["-"], "fstack-protector-strong">, Group,
-  HelpText<"Use a strong heuristic to apply stack protectors to functions">;
+  HelpText<"Enable stack protectors for some functions vulnerable to stack smashing. "
+   "Compared to -fstack-protector, this uses a stronger heuristic "
+   "that includes functions containing arrays of any size (and any type), "
+   "as well as any calls to alloca or the taking of an address from a local variable">;
 def fstack_protector : Flag<["-"], "fstack-protector">, Group,
-  HelpText<"Enable stack protectors for functions potentially vulnerable to stack smashing">;
+  HelpText<"Enable stack protectors for some functions vulnerable to stack smashing. "
+   "This uses a loose heuristic, which considers functions vulnerable "
+   "if they contain a char (or 8bit integer) array or constant sized calls to "
+   "alloca, which are of greater size than ssp-buffer-size (default: 8 bytes). "
+   "All variable sized calls to alloca are considered vulnerable">;
 def fstandalone_debug : Flag<["-"], "fstandalone-debug">, Group, Flags<[CoreOption]>,
   HelpText<"Emit full debug info for all types used by the program">;
 def fno_standalone_debug : Flag<["-"], "fno-standalone-debug">, Group, Flags<[CoreOption]>,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55428: [Docs] Expand -fstack-protector and -fstack-protector-all info

[Carey Williams via Phabricator via cfe-commits]

carwil updated this revision to Diff 177239.
carwil added a comment.

Edited Options.td directly, rather than the generated docs file.


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

https://reviews.llvm.org/D55428

Files:
  include/clang/Driver/Options.td


Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1631,9 +1631,14 @@
 def fstack_protector_all : Flag<["-"], "fstack-protector-all">, Group,
   HelpText<"Force the usage of stack protectors for all functions">;
 def fstack_protector_strong : Flag<["-"], "fstack-protector-strong">, 
Group,
-  HelpText<"Use a strong heuristic to apply stack protectors to functions">;
+  HelpText<"Uses a stronger heuristic to apply stack protectors to functions "
+   "that include arrays of any size (and any type), "
+   "as well as any calls to alloca or the taking of an address from a 
local variable">;
 def fstack_protector : Flag<["-"], "fstack-protector">, Group,
-  HelpText<"Enable stack protectors for functions potentially vulnerable to 
stack smashing">;
+  HelpText<"Enable stack protectors for some functions potentially vulnerable 
to stack smashing. "
+   "Namely those containing a char (or 8bit integer) array or constant 
sized calls to "
+   "alloca, which are of greater size than ssp-buffer-size (default: 8 
bytes). "
+   "All variable sized calls to alloca are considered vulnerable">;
 def fstandalone_debug : Flag<["-"], "fstandalone-debug">, Group, 
Flags<[CoreOption]>,
   HelpText<"Emit full debug info for all types used by the program">;
 def fno_standalone_debug : Flag<["-"], "fno-standalone-debug">, 
Group, Flags<[CoreOption]>,


Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1631,9 +1631,14 @@
 def fstack_protector_all : Flag<["-"], "fstack-protector-all">, Group,
   HelpText<"Force the usage of stack protectors for all functions">;
 def fstack_protector_strong : Flag<["-"], "fstack-protector-strong">, Group,
-  HelpText<"Use a strong heuristic to apply stack protectors to functions">;
+  HelpText<"Uses a stronger heuristic to apply stack protectors to functions "
+   "that include arrays of any size (and any type), "
+   "as well as any calls to alloca or the taking of an address from a local variable">;
 def fstack_protector : Flag<["-"], "fstack-protector">, Group,
-  HelpText<"Enable stack protectors for functions potentially vulnerable to stack smashing">;
+  HelpText<"Enable stack protectors for some functions potentially vulnerable to stack smashing. "
+   "Namely those containing a char (or 8bit integer) array or constant sized calls to "
+   "alloca, which are of greater size than ssp-buffer-size (default: 8 bytes). "
+   "All variable sized calls to alloca are considered vulnerable">;
 def fstandalone_debug : Flag<["-"], "fstandalone-debug">, Group, Flags<[CoreOption]>,
   HelpText<"Emit full debug info for all types used by the program">;
 def fno_standalone_debug : Flag<["-"], "fno-standalone-debug">, Group, Flags<[CoreOption]>,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55428: [Docs] Expand -fstack-protector and -fstack-protector-all info

[Carey Williams via Phabricator via cfe-commits]

carwil created this revision.
carwil added a project: clang.
Herald added a subscriber: cfe-commits.

Improve the description of these command line options by providing specific 
heuristic information, as outlined for the ssp function attribute(s) in LLVM's 
documentation.


Repository:
  rC Clang

https://reviews.llvm.org/D55428

Files:
  docs/ClangCommandLineReference.rst


Index: docs/ClangCommandLineReference.rst
===
--- docs/ClangCommandLineReference.rst
+++ docs/ClangCommandLineReference.rst
@@ -1870,7 +1870,7 @@
 
 .. option:: -fstack-protector, -fno-stack-protector
 
-Enable stack protectors for functions potentially vulnerable to stack smashing
+Enable stack protectors for some functions potentially vulnerable to stack 
smashing. Namely those containing a char (or 8bit integer) array or constant 
sized calls to alloca, which are of greater size than ssp-buffer-size (default: 
8 bytes). All variable sized calls to alloca are considered vulnerable
 
 .. option:: -fstack-protector-all
 
@@ -1878,7 +1878,7 @@
 
 .. option:: -fstack-protector-strong
 
-Use a strong heuristic to apply stack protectors to functions
+Uses a stronger heuristic to apply stack protectors to functions that include 
arrays of any size (and any type), as well as any calls to alloca or the taking 
of an address from a local variable
 
 .. option:: -fstack-size-section, -fno-stack-size-section
 


Index: docs/ClangCommandLineReference.rst
===
--- docs/ClangCommandLineReference.rst
+++ docs/ClangCommandLineReference.rst
@@ -1870,7 +1870,7 @@
 
 .. option:: -fstack-protector, -fno-stack-protector
 
-Enable stack protectors for functions potentially vulnerable to stack smashing
+Enable stack protectors for some functions potentially vulnerable to stack smashing. Namely those containing a char (or 8bit integer) array or constant sized calls to alloca, which are of greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls to alloca are considered vulnerable
 
 .. option:: -fstack-protector-all
 
@@ -1878,7 +1878,7 @@
 
 .. option:: -fstack-protector-strong
 
-Use a strong heuristic to apply stack protectors to functions
+Uses a stronger heuristic to apply stack protectors to functions that include arrays of any size (and any type), as well as any calls to alloca or the taking of an address from a local variable
 
 .. option:: -fstack-size-section, -fno-stack-size-section
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits