[PATCH] D156784: [AArch64][PAC] Declare FPAC subtarget feature

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D156784#4653741 , @atrosinenko 
wrote:

> As discussed in D156716 , it is not clear 
> if I have to add FeatureFPAC to every relevant CPU.

I would say, yes, it has to be added to each CPU that has that feature - that's 
what a subtarget feature is for. If we need to a way to alter code generation
as a response to a user choice, that ought to be done with a specific command 
line option and `TargetOptions` and/or function and module level
attributes.

> Maybe it is worth conservatively assuming that this feature should only be 
> enabled manually by the user as a precaution against "I have CPU core X but 
> it is not listed, so let's use cpu=Y because X supports all the instructions 
> supported by Y //(but not FEAT_FPAC)//" - that would not cause any obvious 
> run-time crashes under normal operation, but would make the code less secure.

As far as I can tell, the existing practice for security-related code 
generation is to have it disabled by default
and enable it explicitly by `clang` command line options (c.f 
`-mbranch-protection=...`, `-mharden-sls=...`, `-fstack-clash-protection`, 
`-fsanitize=memtag`, ...).

In that spirit, I would suggest not using target features to alter code 
generation in a rather obscure way but being quite explicit about it.
For example, have an option `-mauth-ret-check=default|force` where:

- the presence of the option enables LR check before tail calls
- `default` (or `enable`) would mean `FEAT_FPAC` takes precedence
- `force` would mean `FEAT_FPAC` is ignored

Alternatively, maybe even better, these could be options to 
`-mbranch-protection=...`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156784

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

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs passed as function arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/test/CodeGen/aarch64-ABI-align-packed-assembly.c:100
   struct packed_struct on_callee_stack;
   on_callee_stack = va_arg(vl, struct packed_struct);
 }

Can we add some `CHECK:` lines here and to other variadic functions as well (I 
recognize it might not be straightforward)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146242

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

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs passed as function arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Following D148094  , the patch does not apply.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146242

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

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs passed as function arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:5813
 getContext().getTypeUnadjustedAlignInChars(Ty).getQuantity();
-unsigned BaseAlign = getContext().getTypeAlignInChars(Base).getQuantity();
-Align = (Align > BaseAlign && Align >= 16) ? 16 : 0;
+Align = (Align >= 16) ? 16 : 8;
 return ABIArgInfo::getDirect(

The backend ought to set the minimum alignment of a stack slot to 8 anyway (for 
AAPCS), hence setting the minimum here to 8 is redundant.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146242

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

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs passed as function arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

> Previously when a packed struct, containing vector data types such as
> uint16x8_t, is passed as a function argument, the alignment of the
> struct used by the function caller and the alignment used by the callee
> to load the argument from stack does not match.

I would suggest adding tests with assembler output that show what is fixed 
(perhaps pre-committed).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146242

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

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs passed as function arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D146242#4427966 , @tmatheson wrote:

> In D146242#4427707 , @chill wrote:
>
>> I was just thinking to LGTM it :)
>>
>> IMHO, the alignment adjustment  happens because of C.4 (B.3 indeed leave the 
>> HFA/HVA unmodified).
>>
>>> C.4 If the argument is an HFA, an HVA, a Quad-precision Floating-point or 
>>> short vector type then the NSAA is rounded up to the next multiple of 8 if 
>>> its natural alignment is ≤ 8 or the next multiple of 16 if its natural 
>>> alignment is ≥ 16.
>
> I think that C2 would be hit first, suggesting it should be allocated a SIMD 
> register and alignment should be irrelevant, assuming sufficient registers:

Sure, but this is not relevant. We should output a correct `alignstack` 
attribute if in the end it turns out the argument needs to be allocated in 
memory. No harm done if we output the attribute, but the
argument ends up in registers.

>> C.2 If the argument is an HFA or an HVA and there are sufficient unallocated 
>> SIMD and Floating-point registers (NSRN + number of members ≤ 8), then the 
>> argument is allocated to SIMD and Floating-point registers (with one 
>> register per member of the HFA or HVA). The NSRN is incremented by the 
>> number of registers used. The argument has now been allocated.
>
> If not enough registers, the size also needs rounded up:
>
>> C.3 If the argument is an HFA or an HVA then the NSRN is set to 8 and the 
>> size of the argument is rounded up to the nearest multiple of 8 bytes.

I believe that is handled in the backend, by allocating arguments to at least 
8-byte aligned stack slots, e.g. here 
https://github.com/llvm/llvm-project/blob/459f495f49a197a042890e1daa0a98cbae892d2b/llvm/lib/Target/AArch64/AArch64CallingConvention.cpp#L200

> After that C4 would indeed be hit. However C4 differs from B6 
> , in that C4 rounds up to the nearest multiple 
> of 8 or 16 (which is not what the patch currently does) whereas B6 
>  restricts it to either 6 or 16 (which this what 
> this patch does, but shouldn't apply to HVAs).

But there isn't any other power of two between 8 and 16.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146242

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

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs passed as function arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/test/CodeGen/aarch64-ABI-align-packed.c:34
 struct aligned_member_8 {
   uint16x8_t M0 __attribute((aligned (8))); // member alignment 16 since 
__attribute((aligned (n))) sets the minimum alignment
 };

JiruiWu wrote:
> chill wrote:
> > Don't you mean "`__attribute__((aligned(n)))` cannot decrease the minimum 
> > required alignment" ?
> > 
> > 
> I added this comment to explain that the natural alignment of the struct 
> `aligned_member_8` is 16-byte instead of 8-byte. In this test case the 
> alignment of  `M0` is 16 bytes, which is above the minimum required alignment 
> specified by `__attribute__((aligned(8)))`.
Yes, so the `__attribute__` does not actually set the minimum required 
alignment, 
it sets the member alignment to the maximum of the natural and the specified 
alignment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146242

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

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs passed as function arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

I was just thinking to LGTM it :)

IMHO, the alignment adjustment  happens because of C.4 (B.3 indeed leave the 
HFA/HVA unmodified).

> C.4   If the argument is an HFA, an HVA, a Quad-precision Floating-point or 
> short vector type then the NSAA is rounded up to the next multiple of 8 if 
> its natural alignment is ≤ 8 or the next multiple of 16 if its natural 
> alignment is ≥ 16.

Browsing the AAPCS HFA and HVA seem always treated the same, and looking at 
`bool AArch64ABIInfo::isHomogeneousAggregateBaseType(QualType Ty) const` it
recognized both FP types and 64- and 128- bit vectors, so we have uniform 
treatment there as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146242

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

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs passed as function arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/test/CodeGen/aarch64-ABI-align-packed.c:34
 struct aligned_member_8 {
   uint16x8_t M0 __attribute((aligned (8))); // member alignment 16 since 
__attribute((aligned (n))) sets the minimum alignment
 };

Don't you mean "`__attribute__((aligned(n)))` cannot decrease the minimum 
required alignment" ?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146242

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

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs passed as function arguments

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:5809
 
-// For alignment adjusted HFAs, cap the argument alignment to 16, leave it
-// default otherwise.
+// For alignment adjusted HFAs, cap the argument alignment to 16, otherwise
+// set it to 8 according to the AAPCS64 document.

No need to "alignment adjusted", just "HFA/HVA"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146242

___
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] 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] 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 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] 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] 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] D118199: [AArch64] ACLE feature macro for Armv8.8-A MOPS

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D118199#3287636 , @chill wrote:

> I suppose `clang/test/CodeGen/aarch64-mops.c` needs to be run with `clang 
> -march=armv8-a+mops+memtag` (not `clang_cc1`) so it picks up declarations of 
> tagging intrinsics from `arm_acle.h`.

Uhm, scratch that.

For checking diagnostics, `clang_cc1 --verify`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118199

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

[PATCH] D118199: [AArch64] ACLE feature macro for Armv8.8-A MOPS

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

I suppose `clang/test/CodeGen/aarch64-mops.c` needs to be run with `clang 
-march=armv8-a+mops+memtag` (not `clang_cc1`) so it picks up declarations of 
tagging intrinsics from `arm_acle.h`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118199

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

[PATCH] D118199: [AArch64] ACLE feature macro for Armv8.8-A MOPS

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/Basic/Targets/AArch64.cpp:229
 MacroBuilder &Builder) const {
+  // FIXME: this does not handle the case where MOPS is disabled using +nomops
+  Builder.defineMacro("__ARM_FEATURE_MOPS", "1");

What's the deal with `"+nomops"` ? This FIXME sort of contradicts with an 
earlier comment


> Add support for +nomops




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118199

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

[PATCH] D115501: [clang][ARM] Emit warnings when PACBTI-M is used with unsupported architectures

[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, cheers!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115501

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

[PATCH] D115501: [clang][ARM] Emit warnings when PACBTI-M is used with unsupported architectures

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/Basic/Targets/ARM.cpp:387
+
+  return a.isArmT32();
+}

For example Arm7-a defines the T32 instruction set, buy we still want a warning.
Maybe we need `return a.isArmT32() &&  a.isArmMClass()`.
I'm not actually sure whether the triple correctly reflects the target 
instruction set, e.g.
what are we going to get from `-target arm-eabi -march=armv7-a -mthumb`, so the 
approach with
the target triple might not be working.




Comment at: clang/lib/CodeGen/TargetInfo.cpp:6402
+ CGM.getLangOpts().hasSignReturnAddress() ||
+ CGM.getLangOpts().isSignReturnAddressScopeAll()) {
+// If the Branch Protection attribute is missing, validate the target

This condition `CGM.getLangOpts().isSignReturnAddressScopeAll()` is redundant.



Comment at: llvm/include/llvm/ADT/Triple.h:724
 
+  /// Tests whether the target is T32.
+  bool isArmT32() const {

This function does not look quite as expected.

`!isARM()` might be `isThumb()` but we're going to return false, isn't it ?

Then `isThumb()` might be true while we have, say, `armv6k`.

AFAICT, the test (and probably the whole function) ought to be

```
  switch (auto SubArch = getSubArch()) {
  case Triple::ARMSubArch_v8m_baseline,
  case Triple::ARMSubArch_v7s:
  case Triple::ARMSubArch_v7k:
  case Triple::ARMSubArch_v7ve:
  case Triple::ARMSubArch_v6:
  case Triple::ARMSubArch_v6m:
  case Triple::ARMSubArch_v6k:
  case Triple::ARMSubArch_v6t2:
  case Triple::ARMSubArch_v5:
  case Triple::ARMSubArch_v5te:
  case Triple::ARMSubArch_v4t:
return false;
  default:
  return true;
   }
```

which is pretty future-proof.





Comment at: llvm/include/llvm/ADT/Triple.h:725
+  /// Tests whether the target is T32.
+  bool isArmT32() const {
+if (!isARM())

In any case, if we're going to change the `Triple`, it should come with unit 
tests in `unittest/ADT/TripleTest.cpp`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115501

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

[PATCH] D112427: [ARM] Implement setjmp BTI placement for PACBTI-M

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: llvm/lib/Target/ARM/ARMInstrThumb2.td:5745
+ IIC_Br, [(ARMt2CallBTI tglobaladdr:$func)]>,
+ Requires<[IsThumb2]>, Sched<[WriteBrL]>;

DavidSpickett wrote:
> Should this require `IsMClass` instead/also? Though I wasn't able to get 
> anything weird to happen when using an A profile triple so maybe I'm missing 
> a check elsewhere that means you'd never get to this point with A profile Arm.
> 
> For example this A profile triple:
> ```
> $ ./bin/clang --target=thumbv8-arm-none-eabi /tmp/test.c -o /tmp/test.o -o - 
> -S -mbranch-protection=bti -mthumb
> ```
> 
> Doesn't put anything after a call to `setjmp`, nop or otherwise, but I can't 
> place where that decision is made.
The decision is made in ARMMachineFunctionInfo

https://github.com/llvm/llvm-project/blob/a02af37560ff5aa22dcef5735ef25eaf58eaaf64/llvm/lib/Target/ARM/ARMMachineFunctionInfo.cpp#L18


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112427

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

[PATCH] D116160: [AArch64] ACLE feature macro for Armv8.8-A MOPS

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/Basic/Targets/AArch64.cpp:666
 
+  HasMOPS |= ArchKind == llvm::AArch64::ArchKind::ARMV8_8A ||
+ ArchKind == llvm::AArch64::ArchKind::ARMV9_3A;

So, this is enabled by default (as in "is mandatory part") of 8.8-a and 9.3-a? 
Why don't we handle it like other extensions in `AArch64TargetParser.def`  like 
https://reviews.llvm.org/D115694#inline-1110596 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116160

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

[PATCH] D115694: [ARM] Introduce an empty "armv8.8-a" architecture.

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: llvm/include/llvm/Support/ARMTargetParser.def:125
   ARM::AEK_DOTPROD| ARM::AEK_BF16 | ARM::AEK_I8MM))
+ARM_ARCH("armv8.8-a", ARMV8_8A, "8.8-A", "v8.8a",
+ ARMBuildAttrs::CPUArch::v8_A, FK_CRYPTO_NEON_FP_ARMV8,

... here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115694

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

[PATCH] D115501: [clang][ARM] Emit warnings when PACBTI-M is used with unsupported architectures

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/Basic/Targets/ARM.cpp:391-392
 
+  if (!Arch.empty() && !isBranchProtectionSupportedArch(Arch))
+return false;
+

amilendra wrote:
> chill wrote:
> > On empty `Arch` it'd continue down the function, but we'd like to return 
> > failure.
> I am having trouble getting the test `arm-branch-protection-attr-1.c` to work 
> after these changes. `validateBranchProtection()` checks the combination of 
> two parameters, the branch protection attribute and architecture.
> If the architecture is empty, like below, shouldn't the function to continue 
> checking further than simply returning false? 
> ```
> __attribute__((target("branch-protection=bti"))) void btionly() {}
> ```
> Or should I be using something else other than 
> `CGM.getTarget().getTargetOpts().CPU` to get the architecture in 
> `ARMTargetCodeGenInfo::setTargetAttributes`?
> 
We shouldn't be getting an empty `Arch`, or rather we should definitely know 
what we are generating code for.
If that cannot be reliably obtained via wherever the `Arch` parameter comes 
from, maybe we could instead check
target features (`TargetOptions::Features`).  It's conceptually //more 
correct// too, even though in this particular instance
it probably does not matter much.

As a general note too, I think it'd be better to check for when PACBTI-M 
instructions (NOP or not) are definitely *not* available
as architecture names where they are is likely to change with time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115501

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

[PATCH] D115501: [clang][ARM] Emit warnings when PACBTI-M is used with unsupported architectures

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/Basic/Targets/AArch64.h:70
 
-  bool validateBranchProtection(StringRef, BranchProtectionInfo &,
+  bool validateBranchProtection(StringRef, StringRef, BranchProtectionInfo &,
 StringRef &) const override;

Would be nice to have parameter names, like in the adjacent declarations.



Comment at: clang/lib/Basic/Targets/ARM.cpp:375-379
+  if (ArchKind != llvm::ARM::ArchKind::ARMV8_1MMainline &&
+  ArchKind != llvm::ARM::ArchKind::ARMV8MMainline &&
+  ArchKind != llvm::ARM::ArchKind::ARMV7M &&
+  ArchKind != llvm::ARM::ArchKind::ARMV7EM)
+return false;





Comment at: clang/lib/Basic/Targets/ARM.cpp:381
+
+  return true;
+}





Comment at: clang/lib/Basic/Targets/ARM.cpp:391-392
 
+  if (!Arch.empty() && !isBranchProtectionSupportedArch(Arch))
+return false;
+

On empty `Arch` it'd continue down the function, but we'd like to return 
failure.



Comment at: clang/lib/Basic/Targets/ARM.h:128
 
-  bool validateBranchProtection(StringRef, BranchProtectionInfo &,
+  bool isBranchProtectionSupportedArch(StringRef) const override;
+  bool validateBranchProtection(StringRef, StringRef, BranchProtectionInfo &,

Likewise.



Comment at: clang/test/CodeGen/arm_acle.c:1530
 // AArch64-NEXT:ret i32 [[TMP0]]
 //
 uint32_t test_crc32cd(uint32_t a, uint64_t b) {

These look like random changes for the untrained eye


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115501

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

[PATCH] D112421: [clang][ARM] PACBTI-M frontend support

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:6377
+
+static const char *SignReturnAddrStr[] = {"none", "non-leaf", "all"};
+Fn->addFnAttr("sign-return-address",

vhscampos wrote:
> I reckon selecting the string using a switch statement on BPI.SignReturnAddr 
> is more type safe than doing it like this. The current selection is prone to 
> out of bounds accesses to the array in case the enum changes. Please consider 
> so.
Guard it with an assert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112421

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

[PATCH] D112430: [ARM][libunwind] add PACBTI-M support for libunwind

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: libunwind/src/Unwind-EHABI.cpp:312
   uint8_t registers = getByte(data, offset++);
-  if (registers & 0xf0 || !registers)
+  if (registers & 0xf0)
 return _URC_FAILURE;

mstorsjo wrote:
> This particular change looks unrelated to the rest, on a quick glance
Indeed, that change is a leftover from when the encoding of the instruction to 
pop return address authentication code was `<0xb1,0x00>`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112430

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

[PATCH] D112431: [ARM][clang] Define feature test macro for the PACBTI-M extension

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/Basic/Targets/ARM.cpp:897
+Builder.defineMacro("__ARM_FEATURE_PAUTH", "1");
+Builder.defineMacro("__ARM_FEATURE_BTI", "1");
+  }

miyuki wrote:
> Since we decided to have two separate flags, I think this line should be in a 
> separate if statement: `if (HasBTI)`.
Agree. If this ever changes, we'd have to modify only  around lines 550.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112431

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

[PATCH] D112421: [clang][ARM] PACBTI-M frontend support

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/Basic/Targets/AArch64.cpp:134-135
  StringRef &Err) const {
-  llvm::AArch64::ParsedBranchProtection PBP;
-  if (!llvm::AArch64::parseBranchProtection(Spec, PBP, Err))
+  llvm::ARM::ParsedBranchProtection PBP;
+  if (!llvm::ARM::parseBranchProtection(Spec, PBP, Err))
 return false;

danielkiss wrote:
> aaron.ballman wrote:
> > chill wrote:
> > > vhscampos wrote:
> > > > aaron.ballman wrote:
> > > > > This change surprises me. Why should AArch64TargetInfo prefer calling 
> > > > > into ARM instead?
> > > > Since that particular function ended up identical in both ARM and 
> > > > AArch64, we removed the AArch64 specific function and kept only one 
> > > > under ARM. You can spot the removal further down the patch.
> > > > 
> > > > The ARM namespace under ARMTargetParser.h already had code used in 
> > > > AArch64TargetParser, so we did not introduce new cross dependencies.
> > > It's the unfortunate overload of "ARM" used to denote the backend and the 
> > > organisation.
> > Ah, that's good to know, thank you for the explanation. (And yeah, that is 
> > an unfortunate overload of the term.)
> Wondering will this link when the `LLVM_TARGETS_TO_BUILD` does not contains 
> ARM but AArch64?
Should link, the function is in `lib/Support/TragetParser.cpp`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112421

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

[PATCH] D112421: [clang][ARM] PACBTI-M frontend support

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: clang/lib/Basic/Targets/AArch64.cpp:134-135
  StringRef &Err) const {
-  llvm::AArch64::ParsedBranchProtection PBP;
-  if (!llvm::AArch64::parseBranchProtection(Spec, PBP, Err))
+  llvm::ARM::ParsedBranchProtection PBP;
+  if (!llvm::ARM::parseBranchProtection(Spec, PBP, Err))
 return false;

vhscampos wrote:
> aaron.ballman wrote:
> > This change surprises me. Why should AArch64TargetInfo prefer calling into 
> > ARM instead?
> Since that particular function ended up identical in both ARM and AArch64, we 
> removed the AArch64 specific function and kept only one under ARM. You can 
> spot the removal further down the patch.
> 
> The ARM namespace under ARMTargetParser.h already had code used in 
> AArch64TargetParser, so we did not introduce new cross dependencies.
It's the unfortunate overload of "ARM" used to denote the backend and the 
organisation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112421

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

[PATCH] D103080: [CMake] Ignore arm_*.h for non-ARM build

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

IMHO, it's possible to write a frontend test, which includes, say `arm_neon.h`, 
but does not really require the `ARM` or `AArch64` backends to be configured 
(e.g. `CodeGen/arm-vector-align.c`?)
If `arm_neon.h` is not built, then the test would need the appropriate 
`REQUIRES` line, but than that means the frontend test coverage would decrease 
for people, who
are not interested in Arm backends. Mind you, even if a test is Arm specific, 
that does not mean it does not depend on generic code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103080

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

[PATCH] D102693: Do not create LLVM IR `constant`s for objects with dynamic initialisation

[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 rG21aa107eb79f: Reland "Do not create LLVM IR `constant`s 
for objects with dynamic… (authored by chill).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102693

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGenCXX/clang-sections-1.cpp
  clang/test/CodeGenCXX/const-dynamic-init.cpp

Index: clang/test/CodeGenCXX/const-dynamic-init.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/const-dynamic-init.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -o - %s | FileCheck %s
+
+__attribute__((section("A")))
+const int a = 1;
+const int *f() { return &a; }
+// CHECK: @_ZL1a = internal constant i32 1, section "A"
+
+int init();
+__attribute__((section("B")))
+const int b = init();
+// Even if it's const-qualified, it must not be LLVM IR `constant` since it's
+// dynamically initialised.
+// CHECK: @_ZL1b = internal global i32 0, section "B"
+
+__attribute__((section("C")))
+int c = 2;
+// CHECK: @c = {{.*}}global i32 2, section "C"
+
+__attribute__((section("D")))
+int d = init();
+// CHECK: @d = {{.*}}global i32 0, section "D"
+
+__attribute__((section("E")))
+int e;
+// CHECK: @e = {{.*}}global i32 0, section "E", align 4
Index: clang/test/CodeGenCXX/clang-sections-1.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/clang-sections-1.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -o - %s | FileCheck %s --check-prefix=LLVM
+// RUN: %clang_cc1 -triple x86_64-linux -S -o - %s | FileCheck %s --check-prefix=ASM
+// Actually, any ELF target would do
+// REQUIRES: x86_64-linux
+
+#pragma clang section bss = "B$$" data = "d@t@" rodata = "r0d@t@"
+
+const int a = 1;
+const int *f() { return &a; }
+
+int init();
+const int b = init();
+
+int c = 2;
+
+int d = init();
+
+int e;
+
+// LLVM: @_ZL1a = internal constant i32 1, align 4 #[[#A:]]
+// LLVM: @_ZL1b = internal global i32 0, align 4 #[[#A]]
+// LLVM: @c = {{.*}}global i32 2, align 4 #[[#A]]
+// LLVM: @d = {{.*}}global i32 0, align 4 #[[#A]]
+// LLVM: @e = {{.*}}global i32 0, align 4 #[[#A]]
+
+// LLVM: attributes #[[#A]] = { "bss-section"="B$$" "data-section"="d@t@" "rodata-section"="r0d@t@" }
+
+// ASM:   .section "r0d@t@","a",@progbits
+// ASM-NOT:   .section
+// ASM-LABEL: _ZL1a:
+// ASM-NEXT:  .long 1
+
+// ASM:   .section "B$$","aw",@nobits
+// ASM-NOT:   .section
+// ASM-LABEL: _ZL1b:
+// ASM-NEXT: .long 0
+
+// ASM:   .section "d@t@","aw",@progbits
+// ASM-NOT:   .section
+// ASM-LABEL: c:
+// ASM:   .long 2
+
+// ASM:   .section "B$$","aw",@nobits
+// ASM-NOT:   .section
+// ASM-LABEL: d:
+// ASM:   .long 0
+
+// ASM-NOT:   .section
+// ASM-LABEL: e:
+// ASM.long 0
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -13047,43 +13047,6 @@
 }
   }
 
-  // Apply section attributes and pragmas to global variables.
-  bool GlobalStorage = var->hasGlobalStorage();
-  if (GlobalStorage && var->isThisDeclarationADefinition() &&
-  !inTemplateInstantiation()) {
-PragmaStack *Stack = nullptr;
-int SectionFlags = ASTContext::PSF_Read;
-if (var->getType().isConstQualified())
-  Stack = &ConstSegStack;
-else if (!var->getInit()) {
-  Stack = &BSSSegStack;
-  SectionFlags |= ASTContext::PSF_Write;
-} else {
-  Stack = &DataSegStack;
-  SectionFlags |= ASTContext::PSF_Write;
-}
-if (const SectionAttr *SA = var->getAttr()) {
-  if (SA->getSyntax() == AttributeCommonInfo::AS_Declspec)
-SectionFlags |= ASTContext::PSF_Implicit;
-  UnifySection(SA->getName(), SectionFlags, var);
-} else if (Stack->CurrentValue) {
-  SectionFlags |= ASTContext::PSF_Implicit;
-  auto SectionName = Stack->CurrentValue->getString();
-  var->addAttr(SectionAttr::CreateImplicit(
-  Context, SectionName, Stack->CurrentPragmaLocation,
-  AttributeCommonInfo::AS_Pragma, SectionAttr::Declspec_allocate));
-  if (UnifySection(SectionName, SectionFlags, var))
-var->dropAttr();
-}
-
-// Apply the init_seg attribute if this has an initializer.  If the
-// initializer turns out to not be dynamic, we'll end up ignoring this
-// attribute.
-if (CurInitSeg && var->getInit())
-  var->addAttr(InitSegAttr::CreateImplicit(Context, CurInitSeg->getString(),
-   CurInitSegLoc,
-   AttributeCommonInfo::AS_Pragma));
-  }
 
   if (!var->getType()->isStructureType() && var->hasInit() &&
   isa(var->getInit())) {
@@ -13133,14 +13096,6 @@

[PATCH] D102693: Do not create LLVM IR `constant`s for objects with dynamic initialisation

[Momchil Velikov via Phabricator via cfe-commits]

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

Updated a test to run for `x86_64-linux` instead of `%itanium_abi_triple`, to 
avoid having invalid
syntax for MACH-O sections. The patch itself does not care about section 
attribute syntax and x86 target
does not even need to be compiled.


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

https://reviews.llvm.org/D102693

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGenCXX/clang-sections-1.cpp
  clang/test/CodeGenCXX/const-dynamic-init.cpp

Index: clang/test/CodeGenCXX/const-dynamic-init.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/const-dynamic-init.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -o - %s | FileCheck %s
+
+__attribute__((section("A")))
+const int a = 1;
+const int *f() { return &a; }
+// CHECK: @_ZL1a = internal constant i32 1, section "A"
+
+int init();
+__attribute__((section("B")))
+const int b = init();
+// Even if it's const-qualified, it must not be LLVM IR `constant` since it's
+// dynamically initialised.
+// CHECK: @_ZL1b = internal global i32 0, section "B"
+
+__attribute__((section("C")))
+int c = 2;
+// CHECK: @c = {{.*}}global i32 2, section "C"
+
+__attribute__((section("D")))
+int d = init();
+// CHECK: @d = {{.*}}global i32 0, section "D"
+
+__attribute__((section("E")))
+int e;
+// CHECK: @e = {{.*}}global i32 0, section "E", align 4
Index: clang/test/CodeGenCXX/clang-sections-1.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/clang-sections-1.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -o - %s | FileCheck %s --check-prefix=LLVM
+// RUN: %clang_cc1 -triple x86_64-linux -S -o - %s | FileCheck %s --check-prefix=ASM
+// Actually, any ELF target would do
+// REQUIRES: x86_64-linux
+
+#pragma clang section bss = "B$$" data = "d@t@" rodata = "r0d@t@"
+
+const int a = 1;
+const int *f() { return &a; }
+
+int init();
+const int b = init();
+
+int c = 2;
+
+int d = init();
+
+int e;
+
+// LLVM: @_ZL1a = internal constant i32 1, align 4 #[[#A:]]
+// LLVM: @_ZL1b = internal global i32 0, align 4 #[[#A]]
+// LLVM: @c = {{.*}}global i32 2, align 4 #[[#A]]
+// LLVM: @d = {{.*}}global i32 0, align 4 #[[#A]]
+// LLVM: @e = {{.*}}global i32 0, align 4 #[[#A]]
+
+// LLVM: attributes #[[#A]] = { "bss-section"="B$$" "data-section"="d@t@" "rodata-section"="r0d@t@" }
+
+// ASM:   .section "r0d@t@","a",@progbits
+// ASM-NOT:   .section
+// ASM-LABEL: _ZL1a:
+// ASM-NEXT:  .long 1
+
+// ASM:   .section "B$$","aw",@nobits
+// ASM-NOT:   .section
+// ASM-LABEL: _ZL1b:
+// ASM-NEXT: .long 0
+
+// ASM:   .section "d@t@","aw",@progbits
+// ASM-NOT:   .section
+// ASM-LABEL: c:
+// ASM:   .long 2
+
+// ASM:   .section "B$$","aw",@nobits
+// ASM-NOT:   .section
+// ASM-LABEL: d:
+// ASM:   .long 0
+
+// ASM-NOT:   .section
+// ASM-LABEL: e:
+// ASM.long 0
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -13047,43 +13047,6 @@
 }
   }
 
-  // Apply section attributes and pragmas to global variables.
-  bool GlobalStorage = var->hasGlobalStorage();
-  if (GlobalStorage && var->isThisDeclarationADefinition() &&
-  !inTemplateInstantiation()) {
-PragmaStack *Stack = nullptr;
-int SectionFlags = ASTContext::PSF_Read;
-if (var->getType().isConstQualified())
-  Stack = &ConstSegStack;
-else if (!var->getInit()) {
-  Stack = &BSSSegStack;
-  SectionFlags |= ASTContext::PSF_Write;
-} else {
-  Stack = &DataSegStack;
-  SectionFlags |= ASTContext::PSF_Write;
-}
-if (const SectionAttr *SA = var->getAttr()) {
-  if (SA->getSyntax() == AttributeCommonInfo::AS_Declspec)
-SectionFlags |= ASTContext::PSF_Implicit;
-  UnifySection(SA->getName(), SectionFlags, var);
-} else if (Stack->CurrentValue) {
-  SectionFlags |= ASTContext::PSF_Implicit;
-  auto SectionName = Stack->CurrentValue->getString();
-  var->addAttr(SectionAttr::CreateImplicit(
-  Context, SectionName, Stack->CurrentPragmaLocation,
-  AttributeCommonInfo::AS_Pragma, SectionAttr::Declspec_allocate));
-  if (UnifySection(SectionName, SectionFlags, var))
-var->dropAttr();
-}
-
-// Apply the init_seg attribute if this has an initializer.  If the
-// initializer turns out to not be dynamic, we'll end up ignoring this
-// attribute.
-if (CurInitSeg && var->getInit())
-  var->addAttr(InitSegAttr::CreateImplicit(Context, CurInitSeg->getString(),
-   CurInitSegLoc,
-   AttributeCommonInfo::AS_Pragma));
-  }
 
   if (!var->getType()->isStructureType() && var->h

[PATCH] D102693: Do not create LLVM IR `constant`s for objects with dynamic initialisation

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Thanks, I'll have a look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102693

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

[PATCH] D102693: Do not create LLVM IR `constant`s for objects with dynamic initialisation

[Momchil Velikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG13dd65b3a1a3: Do not create LLVM IR `constant`s for objects 
with dynamic initialisation (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/D102693/new/

https://reviews.llvm.org/D102693

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGenCXX/clang-sections-1.cpp
  clang/test/CodeGenCXX/const-dynamic-init.cpp

Index: clang/test/CodeGenCXX/const-dynamic-init.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/const-dynamic-init.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm -o - %s | FileCheck %s
+
+__attribute__((section("A")))
+const int a = 1;
+const int *f() { return &a; }
+// CHECK: @_ZL1a = internal constant i32 1, section "A"
+
+int init();
+__attribute__((section("B")))
+const int b = init();
+// Even if it's const-qualified, it must not be LLVM IR `constant` since it's
+// dynamically initialised.
+// CHECK: @_ZL1b = internal global i32 0, section "B"
+
+__attribute__((section("C")))
+int c = 2;
+// CHECK: @c = {{.*}}global i32 2, section "C"
+
+__attribute__((section("D")))
+int d = init();
+// CHECK: @d = {{.*}}global i32 0, section "D"
+
+__attribute__((section("E")))
+int e;
+// CHECK: @e = {{.*}}global i32 0, section "E", align 4
Index: clang/test/CodeGenCXX/clang-sections-1.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/clang-sections-1.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -o - %s | FileCheck %s --check-prefix=LLVM
+// RUN: %clang_cc1 -triple x86_64-linux -S -o - %s | FileCheck %s --check-prefix=ASM
+// Actually, any ELF target would do
+// REQUIRES: x86_64-linux
+
+#pragma clang section bss = "B$$" data = "d@t@" rodata = "r0d@t@"
+
+const int a = 1;
+const int *f() { return &a; }
+
+int init();
+const int b = init();
+
+int c = 2;
+
+int d = init();
+
+int e;
+
+// LLVM: @_ZL1a = internal constant i32 1, align 4 #[[#A:]]
+// LLVM: @_ZL1b = internal global i32 0, align 4 #[[#A]]
+// LLVM: @c = {{.*}}global i32 2, align 4 #[[#A]]
+// LLVM: @d = {{.*}}global i32 0, align 4 #[[#A]]
+// LLVM: @e = {{.*}}global i32 0, align 4 #[[#A]]
+
+// LLVM: attributes #[[#A]] = { "bss-section"="B$$" "data-section"="d@t@" "rodata-section"="r0d@t@" }
+
+// ASM:   .section "r0d@t@","a",@progbits
+// ASM-NOT:   .section
+// ASM-LABEL: _ZL1a:
+// ASM-NEXT:  .long 1
+
+// ASM:   .section "B$$","aw",@nobits
+// ASM-NOT:   .section
+// ASM-LABEL: _ZL1b:
+// ASM-NEXT: .long 0
+
+// ASM:   .section "d@t@","aw",@progbits
+// ASM-NOT:   .section
+// ASM-LABEL: c:
+// ASM:   .long 2
+
+// ASM:   .section "B$$","aw",@nobits
+// ASM-NOT:   .section
+// ASM-LABEL: d:
+// ASM:   .long 0
+
+// ASM-NOT:   .section
+// ASM-LABEL: e:
+// ASM.long 0
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -13047,43 +13047,6 @@
 }
   }
 
-  // Apply section attributes and pragmas to global variables.
-  bool GlobalStorage = var->hasGlobalStorage();
-  if (GlobalStorage && var->isThisDeclarationADefinition() &&
-  !inTemplateInstantiation()) {
-PragmaStack *Stack = nullptr;
-int SectionFlags = ASTContext::PSF_Read;
-if (var->getType().isConstQualified())
-  Stack = &ConstSegStack;
-else if (!var->getInit()) {
-  Stack = &BSSSegStack;
-  SectionFlags |= ASTContext::PSF_Write;
-} else {
-  Stack = &DataSegStack;
-  SectionFlags |= ASTContext::PSF_Write;
-}
-if (const SectionAttr *SA = var->getAttr()) {
-  if (SA->getSyntax() == AttributeCommonInfo::AS_Declspec)
-SectionFlags |= ASTContext::PSF_Implicit;
-  UnifySection(SA->getName(), SectionFlags, var);
-} else if (Stack->CurrentValue) {
-  SectionFlags |= ASTContext::PSF_Implicit;
-  auto SectionName = Stack->CurrentValue->getString();
-  var->addAttr(SectionAttr::CreateImplicit(
-  Context, SectionName, Stack->CurrentPragmaLocation,
-  AttributeCommonInfo::AS_Pragma, SectionAttr::Declspec_allocate));
-  if (UnifySection(SectionName, SectionFlags, var))
-var->dropAttr();
-}
-
-// Apply the init_seg attribute if this has an initializer.  If the
-// initializer turns out to not be dynamic, we'll end up ignoring this
-// attribute.
-if (CurInitSeg && var->getInit())
-  var->addAttr(InitSegAttr::CreateImplicit(Context, CurInitSeg->getString(),
-   CurInitSegLoc,
-   AttributeCommonInfo::AS_Pragma));
-  }
 
   if (!var->getType()->isStructureType() && var->hasInit() &&
   isa(var->getInit()))

[PATCH] D100853: [clang][AArch32] Correctly align HA arguments when passed on the stack

[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 rG5c7b43aa8298: [clang][AArch32] Correctly align HA arguments 
when passed on the stack (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/D100853/new/

https://reviews.llvm.org/D100853

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/arm-ha-alignstack.c
  llvm/lib/Target/ARM/ARMCallingConv.cpp
  llvm/test/CodeGen/ARM/ha-alignstack-call.ll
  llvm/test/CodeGen/ARM/ha-alignstack.ll

Index: llvm/test/CodeGen/ARM/ha-alignstack.ll
===
--- /dev/null
+++ llvm/test/CodeGen/ARM/ha-alignstack.ll
@@ -0,0 +1,190 @@
+; RUN: llc --mtriple armv7-eabihf %s -o - | FileCheck %s
+
+%struct.S0 = type { [4 x float] }
+%struct.S1 = type { [2 x float] }
+%struct.S2 = type { [4 x float] }
+%struct.D0 = type { [2 x double] }
+%struct.D1 = type { [2 x double] }
+%struct.D2 = type { [4 x double] }
+
+; pass in registers
+define dso_local float @f0_0(double %d0, double %d1, double %d2, double %d3, double %d4, double %d5, %struct.S0 %s.coerce) local_unnamed_addr #0 {
+entry:
+  %s.coerce.fca.0.0.extract = extractvalue %struct.S0 %s.coerce, 0, 0
+  ret float %s.coerce.fca.0.0.extract
+}
+; CHECK-LABEL: f0_0:
+; CHECK:   vmov.f32 s0, s12
+; CHECK-NEXT:  bx   lr
+
+; pass in memory, no memory/regs split
+define dso_local float @f0_1(double %d0, double %d1, double %d2, double %d3, double %d4, double %d5, float %x, %struct.S0 %s.coerce) local_unnamed_addr #0 {
+entry:
+  %s.coerce.fca.0.0.extract = extractvalue %struct.S0 %s.coerce, 0, 0
+  ret float %s.coerce.fca.0.0.extract
+}
+; CHECK-LABEL: f0_1:
+; CHECK:   vldr s0, [sp]
+; CHECK-NEXT:  bx   lr
+
+; pass in memory, alignment 4
+define dso_local float @f0_2(double %d0, double %d1, double %d2, double %d3, double %d4, double %d5, double %d6, double %d7, float %x, %struct.S0 %s.coerce) local_unnamed_addr #0 {
+entry:
+  %s.coerce.fca.0.0.extract = extractvalue %struct.S0 %s.coerce, 0, 0
+  ret float %s.coerce.fca.0.0.extract
+}
+; CHECK-LABEL: f0_2:
+; CHECK:   vldr s0, [sp, #4]
+; CHECK-NEXT:  bx   lr
+
+; pass in registers
+define dso_local float @f1_0(double %d0, double %d1, double %d2, double %d3, double %d4, double %d5, double %d6, %struct.S1 alignstack(8) %s.coerce) local_unnamed_addr #0 {
+entry:
+  %s.coerce.fca.0.0.extract = extractvalue %struct.S1 %s.coerce, 0, 0
+  ret float %s.coerce.fca.0.0.extract
+}
+; CHECK-LABEL: f1_0:
+; CHECK:   vmov.f32 s0, s14
+; CHECK-NEXT:  bx   lr
+
+; pass in memory, no memory/regs split
+define dso_local float @f1_1(double %d0, double %d1, double %d2, double %d3, double %d4, double %d5, double %d6, float %x, %struct.S1 alignstack(8) %s.coerce) local_unnamed_addr #0 {
+entry:
+  %s.coerce.fca.0.0.extract = extractvalue %struct.S1 %s.coerce, 0, 0
+  ret float %s.coerce.fca.0.0.extract
+}
+; CHECK-LABEL: f1_1:
+; CHECK:   vldr s0, [sp]
+; CHECK-NEXT:  bx   lr
+
+; pass in memory, alignment 8
+define dso_local float @f1_2(double %d0, double %d1, double %d2, double %d3, double %d4, double %d5, double %d6, double %d7, float %x, %struct.S1 alignstack(8) %s.coerce) local_unnamed_addr #0 {
+entry:
+  %s.coerce.fca.0.0.extract = extractvalue %struct.S1 %s.coerce, 0, 0
+  ret float %s.coerce.fca.0.0.extract
+}
+; CHECK-LABEL: f1_2:
+; CHECK:   vldr s0, [sp, #8]
+; CHECK-NEXT:  bx   lr
+
+; pass in registers
+define dso_local float @f2_0(double %d0, double %d1, double %d2, double %d3, double %d4, double %d5, %struct.S2 alignstack(8) %s.coerce) local_unnamed_addr #0 {
+entry:
+  %s.coerce.fca.0.0.extract = extractvalue %struct.S2 %s.coerce, 0, 0
+  ret float %s.coerce.fca.0.0.extract
+}
+; CHECK-LABEL: f2_0:
+; CHECK:   vmov.f32 s0, s12
+; CHECK-NEXT:  bx   lr
+
+; pass in memory, no memory/regs split
+define dso_local float @f2_1(double %d0, double %d1, double %d2, double %d3, double %d4, double %d5, float %x, %struct.S2 alignstack(8) %s.coerce) local_unnamed_addr #0 {
+entry:
+  %s.coerce.fca.0.0.extract = extractvalue %struct.S2 %s.coerce, 0, 0
+  ret float %s.coerce.fca.0.0.extract
+}
+; CHECK-LABEL: f2_1:
+; CHECK:   vldr s0, [sp]
+; CHECK-NEXT:  bx   lr
+
+; pass in memory, alignment 8
+define dso_local float @f2_2(double %d0, double %d1, double %d2, double %d3, double %d4, double %d5, double %d6, double %d7, float %x, %struct.S2 alignstack(8) %s.coerce) local_unnamed_addr #0 {
+entry:
+  %s.coerce.fca.0.0.extract = extractvalue %struct.S2 %s.coerce, 0, 0
+  ret float %s.coerce.fca.0.0.extract
+}
+; CHECK-LABEL: f2_2:
+; CHECK:   vldr s0, [sp, #8]
+; CHECK-NEXT:  bx   lr
+
+; pass in registers
+define dso_local double @g0_0(double %d0, double %d1, double %d2, double %d3, double %d4, double %d5, %struct.D0 %s.coerce) local_unnamed_addr #0 {
+entry:
+  %s

[PATCH] D98794: [AArch64] Correctly align HFA arguments when passed on the stack

[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 rGf9d932e6735a: [clang][AArch64] Correctly align HFA arguments 
when passed on the stack (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/D98794/new/

https://reviews.llvm.org/D98794

Files:
  clang/include/clang/CodeGen/CGFunctionInfo.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/aarch64-args-hfa.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/CodeGen/TargetCallingConv.h
  llvm/include/llvm/IR/Argument.h
  llvm/include/llvm/IR/Attributes.h
  llvm/include/llvm/IR/Function.h
  llvm/include/llvm/IR/InstrTypes.h
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/CodeGen/GlobalISel/CallLowering.cpp
  llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
  llvm/lib/IR/Attributes.cpp
  llvm/lib/IR/Function.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/lib/Target/AArch64/AArch64CallingConvention.cpp
  llvm/test/Bitcode/compatibility.ll
  llvm/test/CodeGen/AArch64/arm64-abi-hfa-args.ll

Index: llvm/test/CodeGen/AArch64/arm64-abi-hfa-args.ll
===
--- /dev/null
+++ llvm/test/CodeGen/AArch64/arm64-abi-hfa-args.ll
@@ -0,0 +1,33 @@
+; RUN: llc < %s -mtriple=arm64-none-eabi | FileCheck %s
+
+; Over-aligned HFA argument placed on register - one element per register
+define double @test_hfa_align_arg_reg([2 x double] alignstack(16) %h.coerce) local_unnamed_addr #0 {
+entry:
+; CHECK-LABEL: test_hfa_align_arg_reg:
+; CHECK-NOT: mov
+; CHECK-NOT: ld
+; CHECK: ret
+  %h.coerce.fca.0.extract = extractvalue [2 x double] %h.coerce, 0
+  ret double %h.coerce.fca.0.extract
+}
+
+; Call with over-aligned HFA argument placed on register - one element per register
+define double @test_hfa_align_call_reg() local_unnamed_addr #0 {
+entry:
+; CHECK-LABEL: test_hfa_align_call_reg:
+; CHECK-DAG: fmov  d0, #1.
+; CHECK-DAG: fmov  d1, #2.
+; CHECK: bltest_hfa_align_arg_reg
+  %call = call double @test_hfa_align_arg_reg([2 x double] alignstack(16) [double 1.00e+00, double 2.00e+00])
+  ret double %call
+}
+
+; Over-aligned HFA argument placed on stack - stack round up to alignment
+define double @test_hfa_align_arg_stack(double %d0, double %d1, double %d2, double %d3, double %d4, double %d5, double %d6, double %d7, float %f, [2 x double] alignstack(16) %h.coerce) local_unnamed_addr #0 {
+entry:
+; CHECK-LABEL: test_hfa_align_arg_stack:
+; CHECK:   ldr  d0, [sp, #16]
+; CHECK-NEXT:  ret
+  %h.coerce.fca.0.extract = extractvalue [2 x double] %h.coerce, 0
+  ret double %h.coerce.fca.0.extract
+}
Index: llvm/test/Bitcode/compatibility.ll
===
--- llvm/test/Bitcode/compatibility.ll
+++ llvm/test/Bitcode/compatibility.ll
@@ -550,6 +550,8 @@
 ; CHECK: declare void @f.param.dereferenceable(i8* dereferenceable(4))
 declare void @f.param.dereferenceable_or_null(i8* dereferenceable_or_null(4))
 ; CHECK: declare void @f.param.dereferenceable_or_null(i8* dereferenceable_or_null(4))
+declare void @f.param.stack_align([2 x double] alignstack(16))
+; CHECK: declare void @f.param.stack_align([2 x double] alignstack(16))
 
 ; Functions -- unnamed_addr and local_unnamed_addr
 declare void @f.unnamed_addr() unnamed_addr
Index: llvm/lib/Target/AArch64/AArch64CallingConvention.cpp
===
--- llvm/lib/Target/AArch64/AArch64CallingConvention.cpp
+++ llvm/lib/Target/AArch64/AArch64CallingConvention.cpp
@@ -88,13 +88,8 @@
   }
 
   unsigned Size = LocVT.getSizeInBits() / 8;
-  const Align StackAlign =
-  State.getMachineFunction().getDataLayout().getStackAlignment();
-  const Align OrigAlign = ArgFlags.getNonZeroOrigAlign();
-  const Align Alignment = std::min(OrigAlign, StackAlign);
-
   for (auto &It : PendingMembers) {
-It.convertToMem(State.AllocateStack(Size, std::max(Alignment, SlotAlign)));
+It.convertToMem(State.AllocateStack(Size, SlotAlign));
 State.addLoc(It);
 SlotAlign = Align(1);
   }
@@ -197,7 +192,12 @@
   State.AllocateReg(Reg);
   }
 
-  const Align SlotAlign = Subtarget.isTargetDarwin() ? Align(1) : Align(8);
+  const Align StackAlign =
+  State.getMachineFunction().getDataLayout().getStackAlignment();
+  const Align MemAlign = ArgFlags.getNonZeroMemAlign();
+  Align SlotAlign = std::min(MemAlign, StackAlign);
+  if (!Subtarget.isTargetDarwin())
+SlotAlign = std::max(SlotAlign, Align(8));
 
   return finishStackBlock(PendingMembers, LocVT, ArgFlags, State, SlotAlign);
 }
Index: llvm/lib/IR/Verifier.cpp
===
--- llvm/lib/IR/Verifier.cpp
+

[PATCH] D75903: [AArch64][CodeGen] Fixing stack alignment of HFA arguments on AArch64 PCS

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: llvm/docs/LangRef.rst:1220
+``alignstack()``
+This indicates the alignment that should be considered by the backend when
+assigning this parameter to a stack slot during calling convention

rnk wrote:
> This seems like you are introducing a new meaning to `alignstack`, which 
> according to the comments, only affects function SP alignment, not parameter 
> alignment.
> 
> I'm assuming the reason you can't use the regular `align` attribute is that 
> it is overloaded to mean two things: the alignment of the pointer when 
> applied to a pointer, and the alignment of the argument memory when that 
> pointer argument is marked `byval`. If you want to resolve this ambiguity, it 
> seems like something that should be discussed on llvm-dev with a wider 
> audience.
Sorry, I couldn't quite get it, do you suggest we should be using the `align` 
attribute instead of `alignstack`, if there  are no
(major) objections on the llvm-dev list?

It certainly makes sense to me to use `align` as it already pertains to 
individual argument alignment (even though it's for pointers only now).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75903

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

[PATCH] D94083: [AArch64] Add +pauth archictecture option, allowing the v8.3a pointer authentication extension.

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D94083#2486800 , @fhahn wrote:

> FWIW I think it would be good to have a bit more details in the description 
> for changes such as this, like a link to the public docs for the extension.

I'm sorry, I assumed this information was shared when the extension itself 
((this patch just adds the command line options),
was added to LLVM three years ago in https://reviews.llvm.org/D36517

The public documentation is in the Armv8-A ARM at 
https://developer.arm.com/documentation/ddi0487/latest
in section "D5 .1.5 Pointer authentication in 
AArch64 state"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94083

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

[PATCH] D94083: [AArch64] Add +pauth archictecture option, allowing the v8.3a pointer authentication extension.

[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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94083

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

[PATCH] D94081: [AArch64] Add +flagm archictecture option, allowing the v8.4a flag modification extension.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94081

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

[PATCH] D91438: [AArch64] Define __ARM_FEATURE_{CRC32,ATOMICS}

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91438

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

[PATCH] D85649: [AArch64] PAC/BTI code generation for LLVM generated functions

[Momchil Velikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa88c722e687e: [AArch64] PAC/BTI code generation for LLVM 
generated functions (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/D85649/new/

https://reviews.llvm.org/D85649

Files:
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/aarch64-branch-protection-attr.c
  clang/test/CodeGen/aarch64-sign-return-address.c
  clang/test/CodeGenCXX/aarch64-sign-return-address-static-ctor.cpp
  llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
  llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
  llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.cpp
  llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h
  llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
  llvm/test/CodeGen/AArch64/branch-target-enforcement-indirect-calls.ll
  llvm/test/CodeGen/AArch64/branch-target-enforcement.mir
  llvm/test/CodeGen/AArch64/bti-branch-relaxation.ll
  llvm/test/CodeGen/AArch64/machine-outliner-2fixup-blr-terminator.mir
  llvm/test/CodeGen/AArch64/machine-outliner-bti.mir
  llvm/test/CodeGen/AArch64/machine-outliner-outline-bti.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-0.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-1.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-3.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
  llvm/test/CodeGen/AArch64/pacbti-llvm-generated-funcs-1.ll
  llvm/test/CodeGen/AArch64/pacbti-llvm-generated-funcs-2.ll
  llvm/test/CodeGen/AArch64/pacbti-module-attrs.ll
  llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll

Index: llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll
===
--- llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll
+++ llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll
@@ -1,6 +1,6 @@
 ; RUN: llc -mtriple=aarch64 %s -o - | FileCheck %s
 
-define void @f0() "patchable-function-entry"="0" "branch-target-enforcement" {
+define void @f0() "patchable-function-entry"="0" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: f0:
 ; CHECK-NEXT: .Lfunc_begin0:
 ; CHECK:  // %bb.0:
@@ -12,7 +12,7 @@
 
 ;; -fpatchable-function-entry=1 -mbranch-protection=bti
 ;; For M=0, place the label .Lpatch0 after the initial BTI.
-define void @f1() "patchable-function-entry"="1" "branch-target-enforcement" {
+define void @f1() "patchable-function-entry"="1" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: f1:
 ; CHECK-NEXT: .Lfunc_begin1:
 ; CHECK-NEXT: .cfi_startproc
@@ -28,7 +28,7 @@
 }
 
 ;; -fpatchable-function-entry=2,1 -mbranch-protection=bti
-define void @f2_1() "patchable-function-entry"="1" "patchable-function-prefix"="1" "branch-target-enforcement" {
+define void @f2_1() "patchable-function-entry"="1" "patchable-function-prefix"="1" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: .type f2_1,@function
 ; CHECK-NEXT: .Ltmp0:
 ; CHECK-NEXT:  nop
@@ -50,7 +50,7 @@
 ;; -fpatchable-function-entry=1 -mbranch-protection=bti
 ;; For M=0, don't create .Lpatch0 if the initial instruction is not BTI,
 ;; even if other basic blocks may have BTI.
-define internal void @f1i(i64 %v) "patchable-function-entry"="1" "branch-target-enforcement" {
+define internal void @f1i(i64 %v) "patchable-function-entry"="1" "branch-target-enforcement"="true" {
 ; CHECK-LABEL: f1i:
 ; CHECK-NEXT: .Lfunc_begin3:
 ; CHECK:  // %bb.0:
Index: llvm/test/CodeGen/AArch64/pacbti-module-attrs.ll
===
--- /dev/null
+++ llvm/test/CodeGen/AArch64/pacbti-module-attrs.ll
@@ -0,0 +1,77 @@
+;; RUN: llc -mtriple=aarch64-eabi -mattr=+v8.5a %s -o - | FileCheck %s
+
+declare i32 @g(i32) #5
+
+define i32 @f0(i32 %x) #0 {
+entry:
+  %call = tail call i32 @g(i32 %x) #5
+  %add = add nsw i32 %call, 1
+  ret i32 %add
+}
+;; CHECK-LABEL: f0:
+;; CHECK-NOT:   bti
+;; CHECK-NOT:   pacia
+;; CHECK-NOT:   reta
+
+define i32 @f1(i32 %x) #1 {
+entry:
+  %call = tail call i32 @g(i32 %x) #5
+  %add = add nsw i32 %call, 1
+  ret i32 %add
+}
+;; CHECK-LABEL: f1:
+;; CHECK:   bti c
+;; CHECK-NOT:   reta
+
+define i32 @f2(i32 %x) #2 {
+entry:
+  %call = tail call i32 @g(i32 %x) #5
+  %add = add nsw i32 %call, 1
+  ret i32 %add
+}
+;; CHECK-LABEL: f2:
+;; CHECK:   paciasp
+;; CHECK:   retaa
+
+define i32 @f3(i32 %x) #3 {
+entry:
+  %call = tail call i32 @g(i32 %x) #5
+  %add = add nsw i32 %call, 1
+  ret i32 %add
+}
+;; CHECK-LABEL: f3:
+;

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

[Momchil Velikov via Phabricator via cfe-commits]

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

In D85649  I changed the module flags to be 
always present and have a zero/non-zero value. That's needed during LTO, if a 
flag is present in one module and absent in another,
no error is reported and the existing flags is used in the merged module, 
affecting the codegen for the module that did not initially have the flag.

tl;dr we need to check the value of the flags, not just their existence.


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] D75044: [AArch64] __builtin_return_address for PAuth.

[Momchil Velikov via Phabricator via cfe-commits]

chill accepted this revision.
chill added a comment.

In D75044#2292302 , @danielkiss wrote:

> @chill ping.

Sorry, I thought about committing all PAC/BTI patches together, but there's no 
reason, is there?
So, let's go ahead and commit the two dealing with `__builtin-return_address` .


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] D83004: [UpdateCCTestChecks] Include generated functions if asked

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

Some tests started failing: 
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-ubuntu/builds/9071


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83004

___
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 accepted this revision.
chill added a comment.
This revision is now accepted and ready to land.

LGTM. It'd be nice if we could get someone non-Arm to have a look too. though.


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.

LGTM, as soon as D85649  is accepted (so they 
stay in sync).


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] D81930: [AArch64] Add -mmark-bti-property flag.

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.



Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCAsmInfo.cpp:106
+
+  EmitBTIMarking = MarkBTIProperty.getValue();
 }

No need to the `.getValue()` part.



Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCAsmInfo.h:34
   explicit AArch64MCAsmInfoELF(const Triple &T);
+  bool EmitBTIMarking;
 };

Is there a need for this data member? The option value  does not change over 
time, and the option can be defined in `AArch64TargetStreamer.cpp`.


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 D85649  I suggested a different version of 
module flags, which is a bit nicer to use, e.g. one can say just

  getModuleFlag("sign-return-address-with-bkey") != nullptr

instead of a) checking for the flag presence, b) getting its value and c) 
comparing it to a set of strings, which is
way too verbose.

Thus, the set of module flags are essentially booleans:

- "sign-return-address" when PAC-RET is enabled; it establishes the defaults of 
signing non-leaf functions with the A key
- "sign-return-address-all", modifies the default, established by 
"sign-return-address" to signing all functions, including ones that do not 
spill LR
- "sign-return-address-with-bkey", modifies the default, established by 
"sign-return-address" to signing with the B key.

These are not ABI, so if, in the future, if we do need a set of values, we can 
easily change it.


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

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

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

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 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] 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] 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] 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] 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_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] 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 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] 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] 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: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] 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 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] 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

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

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
+

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

[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] 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 -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] 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] 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] 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] 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

[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] 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 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] D71129: [ARM][CMSE] Implement CMSE attributes

[Momchil Velikov via Phabricator via cfe-commits]

chill marked 4 inline comments as done.
chill added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:1882
+  if (FI.isCmseNSCall())
+FuncAttrs.addAttribute("cmse_nonsecure_call");
+

snidertm wrote:
> Just curious … Does the LLVM backend have a way to extract a StringRef 
> attribute from a CallInst? I know that you can do that with hasFnAttribute(), 
> but I don't see anything for CallInst objects
CallInst *CI = ... ;
   ... = CI->getAttributes().hasFnAttribute("cmse_nonsecure_call"))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71129



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

[PATCH] D71129: [ARM][CMSE] Implement CMSE attributes

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D71129#1956137 , @snidertm wrote:

> Have you already committed support for CMSE attributes to the LLVM backend? 
> Or is that on the way?


The last two CMSE patches are under review: https://reviews.llvm.org/D76518


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71129



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

[PATCH] D71129: [ARM][CMSE] Implement CMSE attributes

[Momchil Velikov via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/AST/Type.h:3622
 bool getNoCfCheck() const { return Bits & NoCfCheckMask; }
 bool getHasRegParm() const { return (Bits >> RegParmOffset) != 0; }
 

snidertm wrote:
> chill wrote:
> > ... here.
> > 
> >bool getHasRegParm() const { return ((Bits & RegParmMask) >> 
> > RegParmOffset) != 0;
> I don't see how this helps. If RegParmOffset is 8 and the CmseNSCall bit is 
> set in Bits, then your proposed getHasRegParm() will return true. Given the 
> above assignment to Bits, we don't know if the CmseNSCall bit was set by 
> cmseNSCall or by regParm.
> 
> MIght I be missing something?
No, it will not return true, because the mask will clear all bits, except bits 
[8-10,13-31].
Bits [13-31] are unused/zero, and in the patch I'm preparing, the RegParmMask 
will be simply 0x700,
so they will be cleared anyway.
CmseNSCall is bit 12, so it will be cleared. Also RegParm + 1 is at most 7, so, 
it cannot overflow into
NoCfCheck of CmseNSCall.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71129



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

[PATCH] D71129: [ARM][CMSE] Implement CMSE attributes

[Momchil Velikov via Phabricator via cfe-commits]

chill marked 3 inline comments as done.
chill added inline comments.



Comment at: clang/include/clang/AST/Type.h:3588
+  NoCallerSavedRegsMask | NoCfCheckMask | CmseNSCallMask),
   RegParmOffset = 8
 }; // Assumed to be the last field

snidertm wrote:
> Shouldn't RegParmOffset be updated to 9, I believe it is used to shift the 
> regParm value so that it encoded in the bits above CmseNSCallMask
Hmm, I think 8 is OK, but we should mask it ...



Comment at: clang/include/clang/AST/Type.h:3622
 bool getNoCfCheck() const { return Bits & NoCfCheckMask; }
 bool getHasRegParm() const { return (Bits >> RegParmOffset) != 0; }
 

... here.

   bool getHasRegParm() const { return ((Bits & RegParmMask) >> RegParmOffset) 
!= 0;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71129



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

[PATCH] D77134: [clang][AARCH64] Add __ARM_FEATURE_{PAC, BTI}_DEFAULT defines

[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, conditional on the dependent patch. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77134



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

[PATCH] D77131: [clang] Move branch-protection from CodeGenOptions to LangOptions

[Momchil Velikov via Phabricator via cfe-commits]

chill added a reviewer: efriedma.
chill added a subscriber: efriedma.
chill added a comment.

Following @efriedma  comment here 
http://lists.llvm.org/pipermail/cfe-dev/2020-March/065017.html LGTM.




Comment at: clang/include/clang/Basic/TargetInfo.h:18
 #include "clang/Basic/AddressSpaces.h"
-#include "clang/Basic/LLVM.h"
 #include "clang/Basic/CodeGenOptions.h"
+#include "clang/Basic/LLVM.h"

This include is probably not needed anymore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77131



___
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/CGCall.cpp:1828
+  if (CodeGenOpts.BranchTargetEnforcement) {
+FuncAttrs.addAttribute("branch-target-enforcement", "true");
+  }

danielkiss wrote:
> chill wrote:
> > I would really prefer to not set values "true" or "false" for the 
> > attribute: we don't really have tri-state logic there 
> > (absent/present-true/present-false), and those values just add some 
> > not-very useful string processing.
> > 
> the attribute will be  "absent" for the runtime emitted function.
How about setting the attribute for LLVM created functions at the time of 
creation, just like Clang created functions
get their attribute at the time of creation?




Comment at: clang/lib/CodeGen/CGCall.cpp:1831
+
+  auto RASignKind = CodeGenOpts.getSignReturnAddress();
+  if (RASignKind != CodeGenOptions::SignReturnAddressScope::None) {

danielkiss wrote:
> chill wrote:
> > What do we get from setting the PACBTI state in the  default function 
> > attributes? We still have 
> > to do a per function processing, we can just as well avoid repeating the 
> > logic, and spare us some
> > adding and potentially removing attributes churn.
> > 
> in the new patch the per function processing will change the attribute only 
> if really need.
Sure, but that's duplication of code/logic, it's a source of countless issues 
"oh, here's the place I should fix that thing ... oh noes,  turns out I have to 
fix ten more ... hope I've found all ..." ;)




Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:200
+if (!F.hasFnAttribute("branch-target-enforcement"))
+  return false;
+Attribute A = F.getFnAttribute("branch-target-enforcement");

chill wrote:
> chill wrote:
> > This should be "true", although the comment might turn out moot.
> > 
> > If we somehow end up with a function, that does not have that attribute, we 
> > should clear the
> > ELF flag.
> > 
> Oh, I see, those are the cases of sanitizer functions, created at LLVM level, 
> that don't have the attribute.
> Please, leave a comment in that sense.
Or, as mentioned in the other comment, check if it's possible to set the 
attribute at the time of creation (from module attributes?).  Tri-state logic 
is added complexity, if it's necessary, it's necessary, but if it's not, better 
make it simpler.


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] D71129: [ARM][CMSE] Implement CMSE attributes

[Momchil Velikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG080d046c91d2: [ARM][CMSE] Implement CMSE attributes 
(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/D71129/new/

https://reviews.llvm.org/D71129

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/AST/TypeProperties.td
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/CodeGen/CGFunctionInfo.h
  clang/lib/AST/TypePrinter.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/AST/ast-dump-arm-attr.c
  clang/test/CodeGen/arm-cmse-attr.c
  clang/test/CodeGen/arm-cmse-call.c
  clang/test/Driver/ropi-rwpi.c
  clang/test/Driver/save-temps.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/arm-cmse.c
  clang/test/Sema/arm-no-cmse.c
  clang/test/SemaCXX/arm-cmse.cpp

Index: clang/test/SemaCXX/arm-cmse.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/arm-cmse.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -triple thumbv8m.base-none-eabi -mcmse -verify %s
+
+extern "C" void foo() __attribute__((cmse_nonsecure_entry)) {}
+
+void bar() __attribute__((cmse_nonsecure_entry)) {} // expected-error{{function type with 'cmse_nonsecure_entry' attribute must have C linkage}}
Index: clang/test/Sema/arm-no-cmse.c
===
--- /dev/null
+++ clang/test/Sema/arm-no-cmse.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -triple thumbv8m.base-none-eabi -verify %s
+
+typedef void (*callback_ns_1t)()
+  __attribute__((cmse_nonsecure_call)); // expected-warning{{'cmse_nonsecure_call' attribute ignored}}
+
+void f()
+  __attribute__((cmse_nonsecure_entry)) {} // expected-warning{{'cmse_nonsecure_entry' attribute ignored}}
Index: clang/test/Sema/arm-cmse.c
===
--- /dev/null
+++ clang/test/Sema/arm-cmse.c
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -triple thumbv8m.base-none-eabi -mcmse -verify %s
+
+typedef void (*callback_ns_1t)() __attribute__((cmse_nonsecure_call));
+typedef void (*callback_1t)();
+typedef void (*callback_ns_2t)() __attribute__((cmse_nonsecure_call));
+typedef void (*callback_2t)();
+
+void foo(callback_ns_1t nsfptr, // expected-error{{functions may not be declared with 'cmse_nonsecure_call' attribute}}
+ callback_1t fptr) __attribute__((cmse_nonsecure_call))
+{
+  callback_1t fp1 = nsfptr; // expected-warning{{incompatible function pointer types initializing 'callback_1t'}}
+  callback_ns_1t fp2 = fptr; // expected-warning{{incompatible function pointer types initializing 'callback_ns_1t'}}
+  callback_2t fp3 = fptr;
+  callback_ns_2t fp4 = nsfptr;
+}
+
+static void bar() __attribute__((cmse_nonsecure_entry)) // expected-warning{{'cmse_nonsecure_entry' cannot be applied to functions with internal linkage}}
+{
+}
+
+typedef void nonsecure_fn_t(int) __attribute__((cmse_nonsecure_call));
+extern nonsecure_fn_t baz; // expected-error{{functions may not be declared with 'cmse_nonsecure_call' attribute}}
+
+int v0 __attribute__((cmse_nonsecure_call)); // expected-warning {{'cmse_nonsecure_call' only applies to function types; type here is 'int'}}
+int v1 __attribute__((cmse_nonsecure_entry)); // expected-warning {{'cmse_nonsecure_entry' attribute only applies to functions}}
+
+void fn0() __attribute__((cmse_nonsecure_entry));
+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}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -39,6 +39,7 @@
 // CHECK-NEXT: Callback (SubjectMatchRule_function)
 // CHECK-NEXT: Capability (SubjectMatchRule_record, SubjectMatchRule_type_alias)
 // CHECK-NEXT: CarriesDependency (SubjectMatchRule_variable_is_parameter, SubjectMatchRule_objc_method, SubjectMatchRule_function)
+// CHECK-NEXT: CmseNSEntry (SubjectMatchRule_function)
 // CHECK-NEXT: Cold (SubjectMatchRule_function)
 // CHECK-NEXT: Common (SubjectMatchRule_variable)
 // CHECK-NEXT: ConstInit (SubjectMatchRule_variable_is_global)
Index: clang/test/Driver/save-temps.c
=

[PATCH] D75109: Apply function attributes through array declarators

[Momchil Velikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6081ccf4a3b6: Apply function attributes through array 
declarators (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/D75109/new/

https://reviews.llvm.org/D75109

Files:
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGen/attr-noreturn.c
  clang/test/Sema/attr-noreturn.c

Index: clang/test/Sema/attr-noreturn.c
===
--- clang/test/Sema/attr-noreturn.c
+++ clang/test/Sema/attr-noreturn.c
@@ -42,3 +42,34 @@
 }
 
 typedef void (*Fun)(void) __attribute__ ((noreturn(2))); // expected-error {{'noreturn' attribute takes no arguments}}
+
+
+typedef void fn_t(void);
+
+fn_t *fp __attribute__((noreturn));
+void __attribute__((noreturn)) f6(int i) {
+  fp();
+}
+
+fn_t *fps[4] __attribute__((noreturn));
+void __attribute__((noreturn)) f7(int i) {
+  fps[i]();
+}
+
+extern fn_t *ifps[] __attribute__((noreturn));
+void __attribute__((noreturn)) f8(int i) {
+  ifps[i]();
+}
+
+void __attribute__((noreturn)) f9(int n) {
+  extern int g9(int, fn_t **);
+  fn_t *fp[n] __attribute__((noreturn));
+  int i = g9(n, fp);
+  fp[i]();
+}
+
+typedef fn_t *fptrs_t[4];
+fptrs_t ps __attribute__((noreturn));
+void __attribute__((noreturn)) f10(int i) {
+  ps[i]();
+}
Index: clang/test/CodeGen/attr-noreturn.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-noreturn.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -S -emit-llvm %s -o - | FileCheck %s
+
+typedef void (*fptrs_t[4])(void);
+fptrs_t p __attribute__((noreturn));
+
+void __attribute__((noreturn)) f() {
+  p[0]();
+}
+// CHECK: call
+// CHECK-NEXT: unreachable
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -6502,6 +6502,7 @@
   Desugar,
   Attributed,
   Parens,
+  Array,
   Pointer,
   BlockPointer,
   Reference,
@@ -6522,6 +6523,10 @@
 } else if (isa(Ty)) {
   T = cast(Ty)->getInnerType();
   Stack.push_back(Parens);
+} else if (isa(Ty) || isa(Ty) ||
+   isa(Ty)) {
+  T = cast(Ty)->getElementType();
+  Stack.push_back(Array);
 } else if (isa(Ty)) {
   T = cast(Ty)->getPointeeType();
   Stack.push_back(Pointer);
@@ -6599,6 +6604,27 @@
   case MacroQualified:
 return wrap(C, cast(Old)->getUnderlyingType(), I);
 
+  case Array: {
+if (const auto *CAT = dyn_cast(Old)) {
+  QualType New = wrap(C, CAT->getElementType(), I);
+  return C.getConstantArrayType(New, CAT->getSize(), CAT->getSizeExpr(),
+CAT->getSizeModifier(),
+CAT->getIndexTypeCVRQualifiers());
+}
+
+if (const auto *VAT = dyn_cast(Old)) {
+  QualType New = wrap(C, VAT->getElementType(), I);
+  return C.getVariableArrayType(
+  New, VAT->getSizeExpr(), VAT->getSizeModifier(),
+  VAT->getIndexTypeCVRQualifiers(), VAT->getBracketsRange());
+}
+
+const auto *IAT = cast(Old);
+QualType New = wrap(C, IAT->getElementType(), I);
+return C.getIncompleteArrayType(New, IAT->getSizeModifier(),
+IAT->getIndexTypeCVRQualifiers());
+  }
+
   case Pointer: {
 QualType New = wrap(C, cast(Old)->getPointeeType(), I);
 return C.getPointerType(New);
___
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: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:200
+if (!F.hasFnAttribute("branch-target-enforcement"))
+  return false;
+Attribute A = F.getFnAttribute("branch-target-enforcement");

chill wrote:
> This should be "true", although the comment might turn out moot.
> 
> If we somehow end up with a function, that does not have that attribute, we 
> should clear the
> ELF flag.
> 
Oh, I see, those are the cases of sanitizer functions, created at LLVM level, 
that don't have the attribute.
Please, leave a comment in that sense.


Repository:
  rG LLVM Github Monorepo

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] D75181: [AArch64] Handle BTI/PAC in case of generated functions.

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

That said, my comments are not of the "over my dead body" kind ;)




Comment at: clang/lib/CodeGen/CGCall.cpp:1828
+  if (CodeGenOpts.BranchTargetEnforcement) {
+FuncAttrs.addAttribute("branch-target-enforcement", "true");
+  }

I would really prefer to not set values "true" or "false" for the attribute: we 
don't really have tri-state logic there (absent/present-true/present-false), 
and those values just add some not-very useful string processing.




Comment at: clang/lib/CodeGen/CGCall.cpp:1831
+
+  auto RASignKind = CodeGenOpts.getSignReturnAddress();
+  if (RASignKind != CodeGenOptions::SignReturnAddressScope::None) {

What do we get from setting the PACBTI state in the  default function 
attributes? We still have 
to do a per function processing, we can just as well avoid repeating the logic, 
and spare us some
adding and potentially removing attributes churn.




Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:200
+if (!F.hasFnAttribute("branch-target-enforcement"))
+  return false;
+Attribute A = F.getFnAttribute("branch-target-enforcement");

This should be "true", although the comment might turn out moot.

If we somehow end up with a function, that does not have that attribute, we 
should clear the
ELF flag.




Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:201-202
+  return false;
+Attribute A = F.getFnAttribute("branch-target-enforcement");
+return !A.isStringAttribute() || A.getValueAsString() == "false";
   })) {

... that kind of string processing, here and elsewhere.



Comment at: llvm/lib/Target/AArch64/AArch64BranchTargets.cpp:62-66
+  Attribute A = F.getFnAttribute("branch-target-enforcement");
+  if (A.isStringAttribute() && A.getValueAsString() == "false")
+return false;
+
+  if (!F.hasFnAttribute("branch-target-enforcement") &&

Isn't there some redundancy with the two calls to `getFnAttribute` and to 
`hasFnAttribute` ?


Repository:
  rG LLVM Github Monorepo

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] D74966: [PATCH] [ARM] Add Cortex-M55 Support for clang and llvm

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


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

https://reviews.llvm.org/D74966



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

[PATCH] D72633: [ARM][MVE] Fix a corner case of checking for MVE-I with -mfpu=none

[Momchil Velikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGda3f2b414ace: [ARM][MVE] Fix a corner case of checking for 
MVE-I with -mfpu=none (authored by chill).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D72633?vs=240836&id=243803#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72633

Files:
  clang/lib/Driver/ToolChains/Arch/ARM.cpp
  clang/test/Driver/arm-mfpu.c
  clang/test/Preprocessor/arm-target-features.c


Index: clang/test/Preprocessor/arm-target-features.c
===
--- clang/test/Preprocessor/arm-target-features.c
+++ clang/test/Preprocessor/arm-target-features.c
@@ -761,8 +761,9 @@
 // CHECK-V81M-MVEFP: #define __ARM_FEATURE_SIMD32 1
 // CHECK-V81M-MVEFP: #define __ARM_FPV5__ 1
 
-// nofp discards mve.fp, but not mve/dsp
-// RUN: %clang -target arm-arm-none-eabi -march=armv8.1-m.main+mve.fp+nofp -x 
c -E -dM %s -o - | FileCheck -match-full-lines 
--check-prefix=CHECK-V81M-MVEFP-NOFP %s
+// fpu=none/nofp discards mve.fp, but not mve/dsp
+// RUN: %clang -target arm-arm-none-eabi -march=armv8.1-m.main+mve.fp+nofp 
   -x c -E -dM %s -o - | FileCheck -match-full-lines 
--check-prefix=CHECK-V81M-MVEFP-NOFP %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv8.1-m.main+mve.fp  
-mfpu=none -x c -E -dM %s -o - | FileCheck -match-full-lines 
--check-prefix=CHECK-V81M-MVEFP-NOFP %s
 // CHECK-V81M-MVEFP-NOFP: #define __ARM_FEATURE_DSP 1
 // CHECK-V81M-MVEFP-NOFP: #define __ARM_FEATURE_MVE 1
 
Index: clang/test/Driver/arm-mfpu.c
===
--- clang/test/Driver/arm-mfpu.c
+++ clang/test/Driver/arm-mfpu.c
@@ -419,6 +419,21 @@
 // CHECK-MVEFP-FPUNONE-DAG: "-target-feature" "-mve.fp"
 // CHECK-MVEFP-FPUNONE-NOT: "-target-feature" "-fpregs"
 
+// RUN: %clang -target arm-none-none-eabi %s 
-march=armv8.1-m.main+mve.fp+nomve -mfpu=none -### -c 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-MVEFP-NOMVE-FPUNONE %s
+// CHECK-MVEFP-NOMVE-FPUNONE-DAG: "-target-feature" "-vfp2sp"
+// CHECK-MVEFP-NOMVE-FPUNONE-DAG: "-target-feature" "-vfp3d16sp"
+// CHECK-MVEFP-NOMVE-FPUNONE-DAG: "-target-feature" "-vfp4d16sp"
+// CHECK-MVEFP-NOMVE-FPUNONE-DAG: "-target-feature" "-fp-armv8d16sp"
+// CHECK-MVEFP-NOMVE-FPUNONE-DAG: "-target-feature" "-fp64"
+// CHECK-MVEFP-NOMVE-FPUNONE-DAG: "-target-feature" "-d32"
+// CHECK-MVEFP-NOMVE-FPUNONE-DAG: "-target-feature" "-neon"
+// CHECK-MVEFP-NOMVE-FPUNONE-DAG: "-target-feature" "-crypto"
+// CHECK-MVEFP-NOMVE-FPUNONE-DAG: "-target-feature" "+dsp"
+// CHECK-MVEFP-NOMVE-FPUNONE-DAG: "-target-feature" "-mve"
+// CHECK-MVEFP-NOMVE-FPUNONE-DAG: "-target-feature" "-mve.fp"
+// CHECK-MVEFP-NOMVE-FPUNONE-DAG: "-target-feature" "-fpregs"
+
 // RUN: %clang -target arm-none-none-eabi %s -march=armv8.1-m.main+mve 
-mfpu=none -### -c 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-MVEI-FPUNONE %s
 // CHECK-MVEI-FPUNONE-DAG: "-target-feature" "-mve.fp"
Index: clang/lib/Driver/ToolChains/Arch/ARM.cpp
===
--- clang/lib/Driver/ToolChains/Arch/ARM.cpp
+++ clang/lib/Driver/ToolChains/Arch/ARM.cpp
@@ -278,6 +278,13 @@
   return ABI;
 }
 
+static bool hasIntegerMVE(const std::vector &F) {
+  auto MVE = llvm::find(llvm::reverse(F), "+mve");
+  auto NoMVE = llvm::find(llvm::reverse(F), "-mve");
+  return MVE != F.rend() &&
+ (NoMVE == F.rend() || std::distance(MVE, NoMVE) > 0);
+}
+
 void arm::getARMTargetFeatures(const Driver &D, const llvm::Triple &Triple,
const ArgList &Args, ArgStringList &CmdArgs,
std::vector &Features, bool ForAS) {
@@ -456,18 +463,13 @@
 
 // Disable all features relating to hardware FP, not already disabled by 
the
 // above call.
-Features.insert(Features.end(), {"-neon", "-crypto", "-dotprod", 
"-fp16fml",
- "-mve", "-mve.fp", "-fpregs"});
+Features.insert(Features.end(),
+{"-dotprod", "-fp16fml", "-mve", "-mve.fp", "-fpregs"});
   } else if (FPUID == llvm::ARM::FK_NONE) {
 // -mfpu=none is *very* similar to -mfloat-abi=soft, only that it should 
not
 // disable MVE-I.
-Features.insert(Features.end(),
-{"-neon", "-crypto", "-dotprod", "-fp16fml", "-mve.fp"});
-// Even though we remove MVE-FP, we still need to check if it was 
originally
-// present among the requested extensions, because it implies MVE-I, which
-// should not be disabled by -mfpu-none.
-if (!llvm::is_contained(Features, "+mve") &&
-!llvm::is_contained(Features, "+mve.fp"))
+Features.insert(Features.end(), {"-dotprod", "-fp16fml", "-mve.fp"});
+if (!hasIntegerMVE(Features))
   Features.emplace_back("-fpregs");