[PATCH] D138143: [FPEnv] Enable strict fp for AArch64 in clang

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:760
+- Strict floating point has been enabled for AArch64, which means that
+  ``-ftrapping-math``, ``-frounding-math``, ``-ffp-model=strict``, and
+  ``-ffp-exception-behaviour=`` are now accepted.

Do we need tests for these command-line options? I only see tests for 
-ffp-exception-behavior= in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138143

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

[PATCH] D112422: [clang][ARM] emit PACBTI-M feature defines

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

LGTM, I assume that the pre-commit test failure is because the bot hasn't 
applied the parent patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112422

___
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

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

In the commit message: s/armclang/clang/




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1832
   // Enable/disable return address signing and indirect branch targets.
   if (Arg *A = Args.getLastArg(options::OPT_msign_return_address_EQ,
options::OPT_mbranch_protection_EQ)) {

This block is duplicating the behaviour of the `CollectARMPACBTIOptions` call 
below.


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] D112420: [clang][ARM] PACBTI-M assembly support

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: llvm/lib/Target/ARM/ARMInstrThumb2.td:4083
 def : t2InstAlias<"csdb$p",   (t2HINT 20, pred:$p), 1>;
+def : t2InstAlias<"pacbti$p r12,lr,sp", (t2HINT 13, pred:$p), 1>;
+def : t2InstAlias<"bti$p", (t2HINT 15, pred:$p), 1>;

Why are these needed in addition to the PACBTIHintSpaceInst instructions below?



Comment at: llvm/lib/Target/ARM/ARMInstrThumb2.td:5674
+
+def t2AUTG  : PACBTIAut<"autg", 0>;
+def t2BXAUT : PACBTIAut<"bxaut", 1>;

I think all of the `AUT` instructions need `hasSideEffects` set, since they can 
raise exceptions.



Comment at: llvm/lib/Target/ARM/ARMInstrThumb2.td:5675
+def t2AUTG  : PACBTIAut<"autg", 0>;
+def t2BXAUT : PACBTIAut<"bxaut", 1>;
+}

This needs `isBranch` set, and a test added to 
test/MC/ARM/implicit-it-generation.s.



Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:6441
+  Mnemonic == "csetm" ||
+  Mnemonic == "autg"  || Mnemonic == "aut"   ||
+  Mnemonic == "bxaut" || Mnemonic == "pacg"  || Mnemonic == "pac" ||

PACG, AUTG and BXAUT can be conditional, so shouldn't be in this list.



Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:6590
+  Mnemonic.startswith("vpt") || Mnemonic.startswith("vpst") ||
+  Mnemonic == "pacg" || Mnemonic == "pac" || Mnemonic == "pacbti" ||
+  Mnemonic == "autg" || Mnemonic == "aut" || Mnemonic == "bxaut" ||

Again, some of these are predicable.



Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:12283
   // FIXME: Unsupported extensions.
-  {ARM::AEK_OS, {}, {}},
+{ARM::AEK_OS, {}, {}},
   {ARM::AEK_IWMMXT, {}, {}},

Unintentional formatting change.



Comment at: llvm/test/MC/ARM/armv8.1m-pacbti-error.s:3
+
+// CHECK: error: invalid instruction
+pac r0, r1, r2

We should also test the cases where PACG/AUTG/BXAUT cannot use PC/SP.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112420

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

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

This change is causing a lot of failures in the address sanitiser tests on the 
2-stage AArch64 buildbots. For example: 
https://lab.llvm.org/buildbot/#/builders/179/builds/1326

I can reproduce the failures on another AArch64 machine, they only happen with 
a 2-stage build, the first stage tests pass fine. I've verified that reverting 
this (and the related test patches) does make the failures go away.

Is there enough detail in the buildbot logs for you to investigate this, or is 
there any extra stuff (logs, intermediate files) I can send you to help? Would 
it be OK to revert this change until these failures are fixed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105169

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

[PATCH] D109157: [ARM] Mitigate the cve-2021-35465 security vulnurability.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1665
+
+CmdArgs.push_back("-mllvm");
+if (A->getOption().matches(options::OPT_mfix_cmse_cve_2021_35465))

ostannard wrote:
> labrinea wrote:
> > ostannard wrote:
> > > Are these optional also being passed through to the linker when doing LTO?
> > No, the mitigation is only performed in the compiler. Also, I believe that 
> > `-flto` and `-mcmse` are incompatible options.
> The mitigation is performed in the backend, which is run from the linker when 
> doing LTO.
> 
> > Also, I believe that -flto and -mcmse are incompatible options.
> 
> Fair enough
Actually, I just checked and these options are accepted together, and I can't 
find any docs saying they are incompatible. Do you have a link to something 
I've missed? Since there isn't already an error, I think we should either fix 
this to work with LTO (my preference), or add an error when using the options 
together, and document that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109157

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

[PATCH] D109157: [ARM] Mitigate the cve-2021-35465 security vulnurability.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1665
+
+CmdArgs.push_back("-mllvm");
+if (A->getOption().matches(options::OPT_mfix_cmse_cve_2021_35465))

labrinea wrote:
> ostannard wrote:
> > Are these optional also being passed through to the linker when doing LTO?
> No, the mitigation is only performed in the compiler. Also, I believe that 
> `-flto` and `-mcmse` are incompatible options.
The mitigation is performed in the backend, which is run from the linker when 
doing LTO.

> Also, I believe that -flto and -mcmse are incompatible options.

Fair enough



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1666
+CmdArgs.push_back("-mllvm");
+if (A->getOption().matches(options::OPT_mfix_cmse_cve_2021_35465))
+  CmdArgs.push_back("-arm-fix-cmse-cve-2021-35465=1");

labrinea wrote:
> SjoerdMeijer wrote:
> > If `-mcpu=cortex-[m33|m35|m55]` was provided, then 
> > `-arm-fix-cmse-cve-2021-35465=1` is already set and we are adding another 
> > option here? For example, for
> > 
> >   -mcpu=cortex-m33 -mcmse -mfix-cmse-cve-2021-35465
> > 
> > I am expecting:
> > 
> >   "-mllvm" "-arm-fix-cmse-cve-2021-35465=1"  "-mllvm" 
> > "-arm-fix-cmse-cve-2021-35465=1" 
> > 
> > and with `-mno-fix-cmse-cve-2021-35465`:
> > 
> >"-mllvm" "-arm-fix-cmse-cve-2021-35465=1"  "-mllvm" 
> > "-arm-fix-cmse-cve-2021-35465=0" 
> > 
> > Probably it's nicer to just pass this once.
> > 
> > Also, in the tests, I think cases are missing for `-mcpu=...` and 
> > `-m[no-]fix-cmse-cve-2021-35465`.
> Your interpretetion is correct. The established policy is "last option wins", 
> but I could make the Driver pass only one option if that's preferable.
I agree with Sjoerd, the ""last option wins" policy should be implemented in 
the driver, and only the winning option passed through to CC1. You can use 
`Args.hasFlag` instead of `getLastArg` here, with the default set based on the 
CPU option.



Comment at: clang/test/Driver/arm-cmse-cve-2021-35465.c:3
+//
+// RUN: %clang --target=arm-arm-none-eabi -march=armv8-m.main %s -### \
+// RUN:   -mcmse -mno-fix-cmse-cve-2021-35465 2>&1 |\

labrinea wrote:
> ostannard wrote:
> > The last few paragraphs of 
> > https://developer.arm.com/support/arm-security-updates/vlldm-instruction-security-vulnerability
> >  claim that this is enabled by default for -march=armv8-m.main in AC6 and 
> > GCC, is there a reason we're not matching that?
> Yes, the inconsistency lies on the fact that GCC implements the mitigation in 
> library code, therefore it is always present for `-march=armv8-m.main`, 
> whereas in llvm there's no such limitation. We've contacted the authors of 
> this page to rectify the documentation.
This still sounds like an inconsistency which will catch out users migrating 
between GCC and clang. I'd prefer that we match GCC's behaviour, though I'd 
also be OK with leaving it like this as long as these defaults are clearly 
documented in ClangCommandLineReference.rst.



Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1564
   .addReg(Reg)
+  .addReg(ARM::CPSR, RegState::ImplicitDefine)
   .add(predOps(ARMCC::AL));

labrinea wrote:
> ostannard wrote:
> > Why are these needed?
> These prevent the reordering with the mitigation sequence. It answers your 
> next question.
Do you mean that this is modeling the effect of this instruction on the CONTROL 
register, to prevent it from being re-ordered with the MRS instruction? If so, 
then this is unrelated to my next question, and could do with a comment because 
I wouldn't expect any CONTROL bits to be part of ARM::CPSR (they are different 
registers).



Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1626
+  // Thumb2ITBlockPass will not recognise the instruction as conditional.
+  BuildMI(MBB, MBBI, DL, TII->get(ARM::t2IT))
+  .addImm(ARMCC::NE)

labrinea wrote:
> ostannard wrote:
> > This pass runs before the final scheduler pass, so is there a risk that the 
> > IT and VMOV instructions could be moved apart? I think it would be safer to 
> > either put the IT instruction inside the inline asm block, or make this a 
> > new pseudo-instruction expanded in the asm printer.
> The use of `.addReg(ARM::CPSR, RegState::ImplicitDefine)` prevents the 
> reordering. There are regression tests in place that check the mitigation 
> sequence ordering.
> 
> Is this what you meant? Where you refering specifically to the case where 
> `!STI->hasFPRegs()`, when we generate inline asm, or to both scenarios?
My concern is that these two instructions (IT and VMOV) have to be adjacent, 
otherwise the IT would apply to some other instruction, but I don't think 
there's anything here which guarantees that. I don't think a regression test is 
enough here, because the re-ordering coul

[PATCH] D109157: [ARM] Mitigate the cve-2021-35465 security vulnurability.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1665
+
+CmdArgs.push_back("-mllvm");
+if (A->getOption().matches(options::OPT_mfix_cmse_cve_2021_35465))

Are these optional also being passed through to the linker when doing LTO?



Comment at: clang/test/Driver/arm-cmse-cve-2021-35465.c:3
+//
+// RUN: %clang --target=arm-arm-none-eabi -march=armv8-m.main %s -### \
+// RUN:   -mcmse -mno-fix-cmse-cve-2021-35465 2>&1 |\

The last few paragraphs of 
https://developer.arm.com/support/arm-security-updates/vlldm-instruction-security-vulnerability
 claim that this is enabled by default for -march=armv8-m.main in AC6 and GCC, 
is there a reason we're not matching that?



Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1564
   .addReg(Reg)
+  .addReg(ARM::CPSR, RegState::ImplicitDefine)
   .add(predOps(ARMCC::AL));

Why are these needed?



Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1626
+  // Thumb2ITBlockPass will not recognise the instruction as conditional.
+  BuildMI(MBB, MBBI, DL, TII->get(ARM::t2IT))
+  .addImm(ARMCC::NE)

This pass runs before the final scheduler pass, so is there a risk that the IT 
and VMOV instructions could be moved apart? I think it would be safer to either 
put the IT instruction inside the inline asm block, or make this a new 
pseudo-instruction expanded in the asm printer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109157

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

[PATCH] D102090: [CMake][ELF] Add -fno-semantic-interposition and -Bsymbolic-functions

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

There is also a test failure on the aarch64 2-stage bot 
(https://lab.llvm.org/buildbot/#/builders/7/builds/2720) which I've bisected to 
this change, so I'll revert it for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102090

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

[PATCH] D98146: OpaquePtr: Turn inalloca into a type attribute

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

I've reverted this (07e46367bae 
) because 
it was causing Bindings/Go/go.test to fail on the buoldbots. Example failure at 
http://lab.llvm.org:8011/#/builders/107/builds/6075.


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

https://reviews.llvm.org/D98146

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

[PATCH] D98253: [clang][ARM] Refactor ComputeLLVMTriple code for ARM

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard accepted this revision.
ostannard 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/D98253/new/

https://reviews.llvm.org/D98253

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

[PATCH] D96007: [AArch64] Enable stack clash protection for AArch64 linux in clang

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard updated this revision to Diff 321689.

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

https://reviews.llvm.org/D96007

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/stack-clash-protection.c
  clang/test/Driver/stack-clash-protection.c


Index: clang/test/Driver/stack-clash-protection.c
===
--- clang/test/Driver/stack-clash-protection.c
+++ clang/test/Driver/stack-clash-protection.c
@@ -21,12 +21,20 @@
 // SCP-ll-win64-NOT: attributes {{.*}} "probe-stack"="inline-asm"
 // SCP-ll-win64: argument unused during compilation: '-fstack-clash-protection'
 
+// RUN: %clang -target aarch64-unknown-linux -fstack-clash-protection -S 
-emit-llvm -o- %s | FileCheck %s -check-prefix=SCP-aarch64
+// SCP-aarch64: attributes {{.*}} "probe-stack"="inline-asm"
+// SCP-aarch64-SAME: "stack-probe-size"="65536"
+
 int foo(int c) {
   int r;
   __asm__("sub %0, %%rsp"
   :
   : "rm"(c)
+#ifdef __aarch64__
+  : "sp");
+#else
   : "rsp");
+#endif
   __asm__("mov %%rsp, %0"
   : "=rm"(r)::);
   return r;
Index: clang/test/CodeGen/stack-clash-protection.c
===
--- clang/test/CodeGen/stack-clash-protection.c
+++ clang/test/CodeGen/stack-clash-protection.c
@@ -3,6 +3,9 @@
 // RUN: %clang_cc1 -triple s390x-linux-gnu -O0 -S -emit-llvm -o- %s 
-fstack-clash-protection | FileCheck %s
 // RUN: %clang_cc1 -triple powerpc64le-linux-gnu -O0 -S -emit-llvm -o- %s 
-fstack-clash-protection | FileCheck %s
 // RUN: %clang_cc1 -triple powerpc64-linux-gnu -O0 -S -emit-llvm -o- %s 
-fstack-clash-protection | FileCheck %s
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -O0 -S -emit-llvm -o- %s 
-fstack-clash-protection | FileCheck %s
+// RUN: %clang_cc1 -triple aarch64_be-linux-gnu -O0 -S -emit-llvm -o- %s 
-fstack-clash-protection | FileCheck %s
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -O0 -S -emit-llvm -o- %s 
-fstack-clash-protection -mstack-probe-size=65536 | FileCheck %s 
--check-prefix=CHECK --check-prefix=LARGE-GUARD
 
 // CHECK: define{{.*}} void @large_stack() #[[A:.*]] {
 void large_stack() {
@@ -23,3 +26,5 @@
 }
 
 // CHECK: attributes #[[A]] = {{.*}} "probe-stack"="inline-asm"
+// LARGE-GUARD-SAME: "stack-probe-size"="65536"
+// CHECK-NOT: "stack-probe-size"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3091,12 +3091,20 @@
 return;
 
   if (!EffectiveTriple.isX86() && !EffectiveTriple.isSystemZ() &&
-  !EffectiveTriple.isPPC64())
+  !EffectiveTriple.isPPC64() && !EffectiveTriple.isAArch64())
 return;
 
   if (Args.hasFlag(options::OPT_fstack_clash_protection,
-   options::OPT_fno_stack_clash_protection, false))
+   options::OPT_fno_stack_clash_protection, false)) {
 CmdArgs.push_back("-fstack-clash-protection");
+
+if (Args.hasArg(options::OPT_mstack_probe_size)) {
+  StringRef Size = Args.getLastArgValue(options::OPT_mstack_probe_size);
+  CmdArgs.push_back(Args.MakeArgString("-mstack-probe-size=" + Size));
+} else if (EffectiveTriple.isAArch64()) {
+  CmdArgs.push_back("-mstack-probe-size=65536");
+}
+  }
 }
 
 static void RenderTrivialAutoVarInitOptions(const Driver &D,
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1664,8 +1664,12 @@
   if (CodeGenOpts.UnwindTables)
 B.addAttribute(llvm::Attribute::UWTable);
 
-  if (CodeGenOpts.StackClashProtector)
+  if (CodeGenOpts.StackClashProtector) {
 B.addAttribute("probe-stack", "inline-asm");
+if (CodeGenOpts.StackProbeSize != 4096)
+  B.addAttribute("stack-probe-size",
+ llvm::utostr(CodeGenOpts.StackProbeSize));
+  }
 
   if (!hasUnwindExceptions(LangOpts))
 B.addAttribute(llvm::Attribute::NoUnwind);


Index: clang/test/Driver/stack-clash-protection.c
===
--- clang/test/Driver/stack-clash-protection.c
+++ clang/test/Driver/stack-clash-protection.c
@@ -21,12 +21,20 @@
 // SCP-ll-win64-NOT: attributes {{.*}} "probe-stack"="inline-asm"
 // SCP-ll-win64: argument unused during compilation: '-fstack-clash-protection'
 
+// RUN: %clang -target aarch64-unknown-linux -fstack-clash-protection -S -emit-llvm -o- %s | FileCheck %s -check-prefix=SCP-aarch64
+// SCP-aarch64: attributes {{.*}} "probe-stack"="inline-asm"
+// SCP-aarch64-SAME: "stack-probe-size"="65536"
+
 int foo(int c) {
   int r;
   __asm__("sub %0, %%rsp"
   :
   : "rm"(c)
+#ifdef __aarch64__
+  : "sp");
+#else
   : "rsp");
+#endif
   __asm__("mov %%rsp, %0"
   : "=rm"(r

[PATCH] D96007: [AArch64] Enable stack clash protection for AArch64 linux in clang

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard created this revision.
ostannard added reviewers: serge-sans-paille, jnspaulsson, bzEq, tnfchris.
Herald added subscribers: danielkiss, kristof.beyls.
ostannard requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This allows the -fstack-clash-protection option to be used for AArch64
Linux.

Linux for AArch64 uses a 64k stack guard, instead of the 4k guard
assumed by the backend, so we set the stack-probe-size function
attribute to configure this, and the -mstack-probe-size= option can be
used to override this.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96007

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/stack-clash-protection.c
  clang/test/Driver/stack-clash-protection.c


Index: clang/test/Driver/stack-clash-protection.c
===
--- clang/test/Driver/stack-clash-protection.c
+++ clang/test/Driver/stack-clash-protection.c
@@ -21,12 +21,20 @@
 // SCP-ll-win64-NOT: attributes {{.*}} "probe-stack"="inline-asm"
 // SCP-ll-win64: argument unused during compilation: '-fstack-clash-protection'
 
+// RUN: %clang -target aarch64-unknown-linux -fstack-clash-protection -S 
-emit-llvm -o- %s | FileCheck %s -check-prefix=SCP-aarch64
+// SCP-aarch64: attributes {{.*}} "probe-stack"="inline-asm"
+// SCP-aarch64-SAME: "stack-probe-size"="65536"
+
 int foo(int c) {
   int r;
   __asm__("sub %0, %%rsp"
   :
   : "rm"(c)
+#ifdef __aarch64__
+  : "sp");
+#else
   : "rsp");
+#endif
   __asm__("mov %%rsp, %0"
   : "=rm"(r)::);
   return r;
Index: clang/test/CodeGen/stack-clash-protection.c
===
--- clang/test/CodeGen/stack-clash-protection.c
+++ clang/test/CodeGen/stack-clash-protection.c
@@ -3,6 +3,9 @@
 // RUN: %clang_cc1 -triple s390x-linux-gnu -O0 -S -emit-llvm -o- %s 
-fstack-clash-protection | FileCheck %s
 // RUN: %clang_cc1 -triple powerpc64le-linux-gnu -O0 -S -emit-llvm -o- %s 
-fstack-clash-protection | FileCheck %s
 // RUN: %clang_cc1 -triple powerpc64-linux-gnu -O0 -S -emit-llvm -o- %s 
-fstack-clash-protection | FileCheck %s
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -O0 -S -emit-llvm -o- %s 
-fstack-clash-protection | FileCheck %s
+// RUN: %clang_cc1 -triple aarch64_be-linux-gnu -O0 -S -emit-llvm -o- %s 
-fstack-clash-protection | FileCheck %s
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -O0 -S -emit-llvm -o- %s 
-fstack-clash-protection -mstack-probe-size=65536 | FileCheck %s 
--check-prefix=CHECK --check-prefix=LARGE-GUARD
 
 // CHECK: define{{.*}} void @large_stack() #[[A:.*]] {
 void large_stack() {
@@ -23,3 +26,5 @@
 }
 
 // CHECK: attributes #[[A]] = {{.*}} "probe-stack"="inline-asm"
+// LARGE-GUARD-SAME: "stack-probe-size"="65536"
+// CHECK-NOT: "stack-probe-size"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3091,12 +3091,20 @@
 return;
 
   if (!EffectiveTriple.isX86() && !EffectiveTriple.isSystemZ() &&
-  !EffectiveTriple.isPPC64())
+  !EffectiveTriple.isPPC64() && !EffectiveTriple.isAArch64())
 return;
 
   if (Args.hasFlag(options::OPT_fstack_clash_protection,
-   options::OPT_fno_stack_clash_protection, false))
+   options::OPT_fno_stack_clash_protection, false)) {
 CmdArgs.push_back("-fstack-clash-protection");
+
+if (Args.hasArg(options::OPT_mstack_probe_size)) {
+  StringRef Size = Args.getLastArgValue(options::OPT_mstack_probe_size);
+  CmdArgs.push_back(Args.MakeArgString("-mstack-probe-size=" + Size));
+} else if (EffectiveTriple.isAArch64()) {
+  CmdArgs.push_back("-mstack-probe-size=65536");
+}
+  }
 }
 
 static void RenderTrivialAutoVarInitOptions(const Driver &D,
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1664,8 +1664,12 @@
   if (CodeGenOpts.UnwindTables)
 B.addAttribute(llvm::Attribute::UWTable);
 
-  if (CodeGenOpts.StackClashProtector)
+  if (CodeGenOpts.StackClashProtector) {
 B.addAttribute("probe-stack", "inline-asm");
+if (CodeGenOpts.StackProbeSize != 4096)
+  B.addAttribute("stack-probe-size",
+ llvm::utostr(CodeGenOpts.StackProbeSize));
+  }
 
   if (!hasUnwindExceptions(LangOpts))
 B.addAttribute(llvm::Attribute::NoUnwind);


Index: clang/test/Driver/stack-clash-protection.c
===
--- clang/test/Driver/stack-clash-protection.c
+++ clang/test/Driver/stack-clash-protection.c
@@ -21,12 +21,20 @@
 // SCP-ll-win64-NOT: attributes {{.*}} "probe-stack"="inline-asm"
 // SCP-ll-win64: argument unused 

[PATCH] D93221: [ARM] Add clang command line support for -mharden-sls=

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

Ok, LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93221

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

[PATCH] D93231: [ARM] Adding v8.7-A command-line support for the ARM target

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard accepted this revision.
ostannard 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/D93231/new/

https://reviews.llvm.org/D93231

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

[PATCH] D93221: [ARM] Add clang command line support for -mharden-sls=

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

Why is this restricted to v7-A or later? The DSB and ISB instructions have 
existed since v6T2 and v6M.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93221

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

[PATCH] D91776: [ARM][AAarch64] Initial command-line support for v8.7-A

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard accepted this revision.
ostannard 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/D91776/new/

https://reviews.llvm.org/D91776

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

[PATCH] D72932: [ARM] Follow AACPS standard for volatile bit-fields access width

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard accepted this revision.
ostannard added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72932

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

[PATCH] D72932: [ARM] Follow AACPS standard for volatile bit-fields access width

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: clang/test/CodeGen/volatile.c:1
-// RUN: %clang_cc1 -triple=%itanium_abi_triple -emit-llvm < %s | FileCheck %s 
-check-prefix CHECK -check-prefix CHECK-IT
+// RUN: %clang_cc1 -triple=%itanium_abi_triple -emit-llvm < %s | FileCheck %s 
-check-prefix CHECK -check-prefixes %volatile_prefix
 // RUN: %clang_cc1 -triple=%ms_abi_triple -emit-llvm < %s | FileCheck %s 
-check-prefix CHECK -check-prefix CHECK-MS

I think it would be better to change this test to use explicit triples, so that 
we're always testing both the ARM and non-ARM behaviour, regardless of the 
default triple.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72932

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

[PATCH] D72932: [ARM] Follow AACPS standard for volatile bit-fields access width

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard accepted this revision.
ostannard 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/D72932/new/

https://reviews.llvm.org/D72932

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

[PATCH] D72932: [ARM] Follow AACPS standard for volatile bit-fields access width

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.def:396
+/// according to the field declaring type width.
+CODEGENOPT(ForceNoAAPCSBitfieldWidth, 1, 0)
+

dnsampaio wrote:
> ostannard wrote:
> > Why is this a negative option, when the one above is positive?
> The enforcing of number of accesses would not be accepted if it was not an 
> opt-in option. This one I expect it should be accepted with a single opt-out 
> option.
My problem is with the name of the option (adding an extra negative just makes 
things more confusing), not with the default value. This could just be called 
`AAPCSBitfieldWidth`, (unless you think the `Force` is adding something), and 
default to true father than false.



Comment at: clang/include/clang/Driver/Options.td:2328
   HelpText<"Follows the AAPCS standard that all volatile bit-field write 
generates at least one load. (ARM only).">;
+def ForceNoAAPCSBitfieldWidth : Flag<["-"], "fno-AAPCSBitfieldWidth">, 
Group,
+  Flags<[DriverOption,CC1Option]>,

ostannard wrote:
> Command-line options are in kebab-case, so this should be something like 
> `fno-aapcs-bitfield-width`. This also applies to the `fAAPCSBitfieldLoad` 
> option above, assuming it's not too late to change that.
> 
> Please also add a positive version of this option (i.e. 
> `faapcs-bitfield-width`).
This still needs a positive version.



Comment at: clang/lib/CodeGen/CGRecordLayout.h:83
 
+  /// The offset within a contiguous run of bitfields that are represented as
+  /// a single "field" within the LLVM struct type. This offset is in bits.

These doc comments are copied from the ones above, they need changing.



Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:279
+lowerUnion();
+return computeVolatileBitfields();
+  }

Returning `void` is confusing (yes I know it was already there), this should be 
a separate `return;` statement.



Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:288
+  appendPaddingBytes(Size);
+  return computeVolatileBitfields();
+}

Same here.



Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:535
+///
+/// Enforcing the width restriction can be disable using
+/// -fno-aapcs-bitfield-width.

s/disable/disabled/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72932

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

[PATCH] D72932: [ARM] Follow AACPS standard for volatile bit-fields access width

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

You haven't addressed my earlier inline comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72932



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

[PATCH] D76291: [Support] Fix formatted_raw_ostream for UTF-8

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe80b81d1cbf8: [Support] Fix formatted_raw_ostream for UTF-8 
(authored by ostannard).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76291

Files:
  clang/test/Analysis/checker-plugins.c
  llvm/include/llvm/Support/FormattedStream.h
  llvm/lib/Support/FormattedStream.cpp
  llvm/test/MC/ARM/lsl-zero.s
  llvm/unittests/Support/formatted_raw_ostream_test.cpp

Index: llvm/unittests/Support/formatted_raw_ostream_test.cpp
===
--- llvm/unittests/Support/formatted_raw_ostream_test.cpp
+++ llvm/unittests/Support/formatted_raw_ostream_test.cpp
@@ -29,4 +29,143 @@
   }
 }
 
+TEST(formatted_raw_ostreamTest, Test_LineColumn) {
+  // Test tracking of line and column numbers in a stream.
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  formatted_raw_ostream C(B);
+
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  C << "a";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(1U, C.getColumn());
+
+  C << "bcdef";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(6U, C.getColumn());
+
+  // '\n' increments line number, sets column to zero.
+  C << "\n";
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  // '\r sets column to zero without changing line number
+  C << "foo\r";
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  // '\t' advances column to the next multiple of 8.
+  // FIXME: If the column number is already a multiple of 8 this will do
+  // nothing, is this behaviour correct?
+  C << "1\t";
+  EXPECT_EQ(8U, C.getColumn());
+  C << "\t";
+  EXPECT_EQ(8U, C.getColumn());
+  C << "1234567\t";
+  EXPECT_EQ(16U, C.getColumn());
+  EXPECT_EQ(1U, C.getLine());
+}
+
+TEST(formatted_raw_ostreamTest, Test_Flush) {
+  // Flushing the buffer causes the characters in the buffer to be scanned
+  // before the buffer is emptied, so line and column numbers will still be
+  // tracked properly.
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(32);
+  formatted_raw_ostream C(B);
+
+  C << "\nabc";
+  EXPECT_EQ(4U, C.GetNumBytesInBuffer());
+  C.flush();
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(3U, C.getColumn());
+  EXPECT_EQ(0U, C.GetNumBytesInBuffer());
+}
+
+TEST(formatted_raw_ostreamTest, Test_UTF8) {
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(32);
+  formatted_raw_ostream C(B);
+
+  // U+00A0 Non-breaking space: encoded as two bytes, but only one column wide.
+  C << u8"\u00a0";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(1U, C.getColumn());
+  EXPECT_EQ(2U, C.GetNumBytesInBuffer());
+
+  // U+2468 CIRCLED DIGIT NINE: encoded as three bytes, but only one column
+  // wide.
+  C << u8"\u2468";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(2U, C.getColumn());
+  EXPECT_EQ(5U, C.GetNumBytesInBuffer());
+
+  // U+0001 LINEAR B SYLLABLE B008 A: encoded as four bytes, but only one
+  // column wide.
+  C << u8"\U0001";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(3U, C.getColumn());
+  EXPECT_EQ(9U, C.GetNumBytesInBuffer());
+
+  // U+55B5, CJK character, encodes as three bytes, takes up two columns.
+  C << u8"\u55b5";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(5U, C.getColumn());
+  EXPECT_EQ(12U, C.GetNumBytesInBuffer());
+
+  // U+200B, zero-width space, encoded as three bytes but has no effect on the
+  // column or line number.
+  C << u8"\u200b";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(5U, C.getColumn());
+  EXPECT_EQ(15U, C.GetNumBytesInBuffer());
+}
+
+TEST(formatted_raw_ostreamTest, Test_UTF8Buffered) {
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(4);
+  formatted_raw_ostream C(B);
+
+  // U+2468 encodes as three bytes, so will cause the buffer to be flushed after
+  // the first byte (4 byte buffer, 3 bytes already written). We need to save
+  // the first part of the UTF-8 encoding until after the buffer is cleared and
+  // the remaining two bytes are written, at which point we can check the
+  // display width. In this case the display width is 1, so we end at column 4,
+  // with 6 bytes written into total, 2 of which are in the buffer.
+  C << u8"123\u2468";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(4U, C.getColumn());
+  EXPECT_EQ(2U, C.GetNumBytesInBuffer());
+  C.flush();
+  EXPECT_EQ(6U, A.size());
+
+  // Same as above, but with a CJK character which displays as two columns.
+  C << u8"123\u55b5";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(9U, C.getColumn());
+  EXPECT_EQ(2U, C.GetNumBytesInBuffer());
+  C.flush();
+  EXPECT_EQ(12U, A.size());
+}
+
+TEST(formatted_raw_ostreamTest, Test_UTF8TinyBuffer) {
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(1);
+  formatted_raw_ostream C(B);
+
+  // The stream has a one-byte buffer, so 

[PATCH] D76291: [Support] Fix formatted_raw_ostream for UTF-8

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe80b81d1cbf8: [Support] Fix formatted_raw_ostream for UTF-8 
(authored by ostannard).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76291

Files:
  clang/test/Analysis/checker-plugins.c
  llvm/include/llvm/Support/FormattedStream.h
  llvm/lib/Support/FormattedStream.cpp
  llvm/test/MC/ARM/lsl-zero.s
  llvm/unittests/Support/formatted_raw_ostream_test.cpp

Index: llvm/unittests/Support/formatted_raw_ostream_test.cpp
===
--- llvm/unittests/Support/formatted_raw_ostream_test.cpp
+++ llvm/unittests/Support/formatted_raw_ostream_test.cpp
@@ -29,4 +29,143 @@
   }
 }
 
+TEST(formatted_raw_ostreamTest, Test_LineColumn) {
+  // Test tracking of line and column numbers in a stream.
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  formatted_raw_ostream C(B);
+
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  C << "a";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(1U, C.getColumn());
+
+  C << "bcdef";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(6U, C.getColumn());
+
+  // '\n' increments line number, sets column to zero.
+  C << "\n";
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  // '\r sets column to zero without changing line number
+  C << "foo\r";
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  // '\t' advances column to the next multiple of 8.
+  // FIXME: If the column number is already a multiple of 8 this will do
+  // nothing, is this behaviour correct?
+  C << "1\t";
+  EXPECT_EQ(8U, C.getColumn());
+  C << "\t";
+  EXPECT_EQ(8U, C.getColumn());
+  C << "1234567\t";
+  EXPECT_EQ(16U, C.getColumn());
+  EXPECT_EQ(1U, C.getLine());
+}
+
+TEST(formatted_raw_ostreamTest, Test_Flush) {
+  // Flushing the buffer causes the characters in the buffer to be scanned
+  // before the buffer is emptied, so line and column numbers will still be
+  // tracked properly.
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(32);
+  formatted_raw_ostream C(B);
+
+  C << "\nabc";
+  EXPECT_EQ(4U, C.GetNumBytesInBuffer());
+  C.flush();
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(3U, C.getColumn());
+  EXPECT_EQ(0U, C.GetNumBytesInBuffer());
+}
+
+TEST(formatted_raw_ostreamTest, Test_UTF8) {
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(32);
+  formatted_raw_ostream C(B);
+
+  // U+00A0 Non-breaking space: encoded as two bytes, but only one column wide.
+  C << u8"\u00a0";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(1U, C.getColumn());
+  EXPECT_EQ(2U, C.GetNumBytesInBuffer());
+
+  // U+2468 CIRCLED DIGIT NINE: encoded as three bytes, but only one column
+  // wide.
+  C << u8"\u2468";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(2U, C.getColumn());
+  EXPECT_EQ(5U, C.GetNumBytesInBuffer());
+
+  // U+0001 LINEAR B SYLLABLE B008 A: encoded as four bytes, but only one
+  // column wide.
+  C << u8"\U0001";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(3U, C.getColumn());
+  EXPECT_EQ(9U, C.GetNumBytesInBuffer());
+
+  // U+55B5, CJK character, encodes as three bytes, takes up two columns.
+  C << u8"\u55b5";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(5U, C.getColumn());
+  EXPECT_EQ(12U, C.GetNumBytesInBuffer());
+
+  // U+200B, zero-width space, encoded as three bytes but has no effect on the
+  // column or line number.
+  C << u8"\u200b";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(5U, C.getColumn());
+  EXPECT_EQ(15U, C.GetNumBytesInBuffer());
+}
+
+TEST(formatted_raw_ostreamTest, Test_UTF8Buffered) {
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(4);
+  formatted_raw_ostream C(B);
+
+  // U+2468 encodes as three bytes, so will cause the buffer to be flushed after
+  // the first byte (4 byte buffer, 3 bytes already written). We need to save
+  // the first part of the UTF-8 encoding until after the buffer is cleared and
+  // the remaining two bytes are written, at which point we can check the
+  // display width. In this case the display width is 1, so we end at column 4,
+  // with 6 bytes written into total, 2 of which are in the buffer.
+  C << u8"123\u2468";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(4U, C.getColumn());
+  EXPECT_EQ(2U, C.GetNumBytesInBuffer());
+  C.flush();
+  EXPECT_EQ(6U, A.size());
+
+  // Same as above, but with a CJK character which displays as two columns.
+  C << u8"123\u55b5";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(9U, C.getColumn());
+  EXPECT_EQ(2U, C.GetNumBytesInBuffer());
+  C.flush();
+  EXPECT_EQ(12U, A.size());
+}
+
+TEST(formatted_raw_ostreamTest, Test_UTF8TinyBuffer) {
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(1);
+  formatted_raw_ostream C(B);
+
+  // The stream has a one-byte buffer, so it gets flushed multiple times while
+  // printing a single Unicode character.
+  

[PATCH] D76291: [Support] Fix formatted_raw_ostream for UTF-8

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard updated this revision to Diff 275029.
ostannard marked 5 inline comments as done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76291

Files:
  clang/test/Analysis/checker-plugins.c
  llvm/include/llvm/Support/FormattedStream.h
  llvm/lib/Support/FormattedStream.cpp
  llvm/test/MC/ARM/lsl-zero.s
  llvm/unittests/Support/formatted_raw_ostream_test.cpp

Index: llvm/unittests/Support/formatted_raw_ostream_test.cpp
===
--- llvm/unittests/Support/formatted_raw_ostream_test.cpp
+++ llvm/unittests/Support/formatted_raw_ostream_test.cpp
@@ -29,4 +29,143 @@
   }
 }
 
+TEST(formatted_raw_ostreamTest, Test_LineColumn) {
+  // Test tracking of line and column numbers in a stream.
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  formatted_raw_ostream C(B);
+
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  C << "a";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(1U, C.getColumn());
+
+  C << "bcdef";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(6U, C.getColumn());
+
+  // '\n' increments line number, sets column to zero.
+  C << "\n";
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  // '\r sets column to zero without changing line number
+  C << "foo\r";
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  // '\t' advances column to the next multiple of 8.
+  // FIXME: If the column number is already a multiple of 8 this will do
+  // nothing, is this behaviour correct?
+  C << "1\t";
+  EXPECT_EQ(8U, C.getColumn());
+  C << "\t";
+  EXPECT_EQ(8U, C.getColumn());
+  C << "1234567\t";
+  EXPECT_EQ(16U, C.getColumn());
+  EXPECT_EQ(1U, C.getLine());
+}
+
+TEST(formatted_raw_ostreamTest, Test_Flush) {
+  // Flushing the buffer causes the characters in the buffer to be scanned
+  // before the buffer is emptied, so line and column numbers will still be
+  // tracked properly.
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(32);
+  formatted_raw_ostream C(B);
+
+  C << "\nabc";
+  EXPECT_EQ(4U, C.GetNumBytesInBuffer());
+  C.flush();
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(3U, C.getColumn());
+  EXPECT_EQ(0U, C.GetNumBytesInBuffer());
+}
+
+TEST(formatted_raw_ostreamTest, Test_UTF8) {
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(32);
+  formatted_raw_ostream C(B);
+
+  // U+00A0 Non-breaking space: encoded as two bytes, but only one column wide.
+  C << u8"\u00a0";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(1U, C.getColumn());
+  EXPECT_EQ(2U, C.GetNumBytesInBuffer());
+
+  // U+2468 CIRCLED DIGIT NINE: encoded as three bytes, but only one column
+  // wide.
+  C << u8"\u2468";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(2U, C.getColumn());
+  EXPECT_EQ(5U, C.GetNumBytesInBuffer());
+
+  // U+0001 LINEAR B SYLLABLE B008 A: encoded as four bytes, but only one
+  // column wide.
+  C << u8"\U0001";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(3U, C.getColumn());
+  EXPECT_EQ(9U, C.GetNumBytesInBuffer());
+
+  // U+55B5, CJK character, encodes as three bytes, takes up two columns.
+  C << u8"\u55b5";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(5U, C.getColumn());
+  EXPECT_EQ(12U, C.GetNumBytesInBuffer());
+
+  // U+200B, zero-width space, encoded as three bytes but has no effect on the
+  // column or line number.
+  C << u8"\u200b";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(5U, C.getColumn());
+  EXPECT_EQ(15U, C.GetNumBytesInBuffer());
+}
+
+TEST(formatted_raw_ostreamTest, Test_UTF8Buffered) {
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(4);
+  formatted_raw_ostream C(B);
+
+  // U+2468 encodes as three bytes, so will cause the buffer to be flushed after
+  // the first byte (4 byte buffer, 3 bytes already written). We need to save
+  // the first part of the UTF-8 encoding until after the buffer is cleared and
+  // the remaining two bytes are written, at which point we can check the
+  // display width. In this case the display width is 1, so we end at column 4,
+  // with 6 bytes written into total, 2 of which are in the buffer.
+  C << u8"123\u2468";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(4U, C.getColumn());
+  EXPECT_EQ(2U, C.GetNumBytesInBuffer());
+  C.flush();
+  EXPECT_EQ(6U, A.size());
+
+  // Same as above, but with a CJK character which displays as two columns.
+  C << u8"123\u55b5";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(9U, C.getColumn());
+  EXPECT_EQ(2U, C.GetNumBytesInBuffer());
+  C.flush();
+  EXPECT_EQ(12U, A.size());
+}
+
+TEST(formatted_raw_ostreamTest, Test_UTF8TinyBuffer) {
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(1);
+  formatted_raw_ostream C(B);
+
+  // The stream has a one-byte buffer, so it gets flushed multiple times while
+  // printing a single Unicode character.
+  C << u8"\u2468";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(1U, C.getColumn());
+  EXP

[PATCH] D75169: [ARM] Supporting lowering of half-precision FP arguments and returns in AArch32's backend

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

I don't think it makes sense to make `f16` legal for targets which don't have 
any arithmetic operations on it, since that would be contrary to the definition 
of "legal". I'd also expect doing so to introduce a lot more complexity than 
this patch.

I think all of the previous comments have been addressed now, and this LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75169



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

[PATCH] D81428: [ARM] Moving CMSE handling of half arguments and return to the backend

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard accepted this revision.
ostannard 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/D81428/new/

https://reviews.llvm.org/D81428



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

[PATCH] D81428: [ARM] Moving CMSE handling of half arguments and return to the backend

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

Is this expected to work for the soft-float calling convention, or is clang 
still passing half-precision values as integer types for that? If the former, 
then this needs some tests for that case.




Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:2267
 
+// Mask f16 arguments if this is a CMSE nonsecure call
+auto ArgVT = Outs[realArgIdx].ArgVT;

Could this be done more efficiently by changing the ANY_EXTEND above to a 
ZERO_EXTEND when this is a CMSE call?



Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:2955
+
+// Mask f16 arguments if this is a CMSE nonsecure entry
+auto RetVT = Outs[realRVLocIdx].ArgVT;

Again, could this be done by using ZERO_EXTEND instead of ANY_EXTEND for CMSE 
entry functions?



Comment at: llvm/test/CodeGen/ARM/cmse-clear-float-hard.ll:812
 
+define arm_aapcs_vfpcc half @h1(half (half)* nocapture %hptr) #10 {
+; CHECK-8M-LABEL: h1:

The function attributes (`"cmse_nonsecure_entry" nounwind` in this case) can be 
placed here (replacing the `#10`) to make the tests easier to read.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81428



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

[PATCH] D81451: [ARM][Clang] Removing lowering of half-precision FP arguments and returns from Clang's CodeGen

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

LGTM, thanks for working on this patch series, this looks much clearer than 
doing it in clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81451



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

[PATCH] D81404: [AArch64] Add clang command line support for -mharden-sls=

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

LGTM, inline comment could be done as a follow-up patch given that this is 
time-sensitive (recently published security vulnerability).




Comment at: clang/lib/Driver/ToolChains/Arch/AArch64.cpp:229
+  Scope.split(Opts, ",");
+  for (int I = 0, E = Opts.size(); I != E; ++I) {
+StringRef Opt = Opts[I].trim();

Could this be a range-based for loop?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81404



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

[PATCH] D80928: [BFloat] Add convert/copy instrinsic support

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: llvm/include/llvm/IR/IntrinsicsARM.td:789
 
+def int_arm_neon_vcvtfp2bf
+: Intrinsic<[llvm_anyvector_ty], [llvm_anyvector_ty], [IntrNoMem]>;

I only see this being used for f32 -> bf16 conversion, so could this have 
concrete types, instead of llvm_anyvector_ty?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80928



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

[PATCH] D79711: [ARM] Add poly64_t on AArch32.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

Ok, LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79711



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

[PATCH] D79711: [ARM] Add poly64_t on AArch32.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

Should `poly128_t` be available on AArch32 too? I don't see anything in the 
ACLE version you linked restricting it to AArch64 only, and the intrinsics 
reference has a number of intrinsics available for both ISAs using it.




Comment at: clang/include/clang/Basic/TargetBuiltins.h:160
   EltType ET = getEltType();
-  return ET == Poly8 || ET == Poly16;
+  return ET == Poly8 || ET == Poly16 || ET == Poly64;
 }

Should `Poly128` be in this list too?



Comment at: clang/lib/Sema/SemaType.cpp:7658
 } else {
-  // AArch32 polynomial vector are signed.
+  // AArch32 polynomial vectors are signed.
   return BTy->getKind() == BuiltinType::SChar ||

The version of the ACLE you linked says all polynomial types are unsigned, not 
mentioning any difference between AArch32 and AArch64:

  poly8_t, poly16_t, poly64_t and poly128_t are defined as unsigned integer 
types. It is unspecified whether these are the same type as uint8_t, uint16_t, 
uint64_t and uint128_t for overloading and mangling purposes.

Maybe this is something left over from an old version of the ACLE which needs 
updating now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79711



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

[PATCH] D79721: [Clang][AArch64] Capturing proper pointer alignment for Neon vld1 intrinsicts

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard accepted this revision.
ostannard 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/D79721/new/

https://reviews.llvm.org/D79721



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

[PATCH] D76291: [Support] Fix formatted_raw_ostream for UTF-8

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a reviewer: kristof.beyls.
ostannard added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76291



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

[PATCH] D76291: [Support] Fix formatted_raw_ostream for UTF-8

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76291



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

[PATCH] D76291: [Support] Fix formatted_raw_ostream for UTF-8

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard updated this revision to Diff 251105.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76291

Files:
  clang/test/Analysis/checker-plugins.c
  llvm/include/llvm/Support/FormattedStream.h
  llvm/lib/Support/FormattedStream.cpp
  llvm/test/MC/ARM/lsl-zero.s
  llvm/unittests/Support/formatted_raw_ostream_test.cpp

Index: llvm/unittests/Support/formatted_raw_ostream_test.cpp
===
--- llvm/unittests/Support/formatted_raw_ostream_test.cpp
+++ llvm/unittests/Support/formatted_raw_ostream_test.cpp
@@ -29,4 +29,143 @@
   }
 }
 
+TEST(formatted_raw_ostreamTest, Test_LineColumn) {
+  // Test tracking of line and column numbers in a stream.
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  formatted_raw_ostream C(B);
+
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  C << "a";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(1U, C.getColumn());
+
+  C << "bcdef";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(6U, C.getColumn());
+
+  // '\n' increments line number, sets column to zero.
+  C << "\n";
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  // '\r sets column to zero without changing line number
+  C << "foo\r";
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  // '\t' advances column to the next multiple of 8.
+  // FIXME: If the column number is already a multiple of 8 this will do
+  // nothing, is this behaviour correct?
+  C << "1\t";
+  EXPECT_EQ(8U, C.getColumn());
+  C << "\t";
+  EXPECT_EQ(8U, C.getColumn());
+  C << "1234567\t";
+  EXPECT_EQ(16U, C.getColumn());
+  EXPECT_EQ(1U, C.getLine());
+}
+
+TEST(formatted_raw_ostreamTest, Test_Flush) {
+  // Flushing the buffer causes the characters in the buffer to be scanned
+  // before the buffer is emptied, so line and column numbers will still be
+  // tracked properly.
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(32);
+  formatted_raw_ostream C(B);
+
+  C << "\nabc";
+  EXPECT_EQ(4U, C.GetNumBytesInBuffer());
+  C.flush();
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(3U, C.getColumn());
+  EXPECT_EQ(0U, C.GetNumBytesInBuffer());
+}
+
+TEST(formatted_raw_ostreamTest, Test_UTF8) {
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(32);
+  formatted_raw_ostream C(B);
+
+  // U+00A0 Non-breaking space: encoded as two bytes, but only one column wide.
+  C << u8"\u00a0";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(1U, C.getColumn());
+  EXPECT_EQ(2U, C.GetNumBytesInBuffer());
+
+  // U+2468 CIRCLED DIGIT NINE: encoded as three bytes, but only one column
+  // wide.
+  C << u8"\u2468";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(2U, C.getColumn());
+  EXPECT_EQ(5U, C.GetNumBytesInBuffer());
+
+  // U+0001 LINEAR B SYLLABLE B008 A: encoded as four bytes, but only one
+  // column wide.
+  C << u8"\U0001";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(3U, C.getColumn());
+  EXPECT_EQ(9U, C.GetNumBytesInBuffer());
+
+  // U+55B5, CJK character, encodes as three bytes, takes up two columns.
+  C << u8"\u55b5";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(5U, C.getColumn());
+  EXPECT_EQ(12U, C.GetNumBytesInBuffer());
+
+  // U+200B, zero-width space, encoded as three bytes but has no effect on the
+  // column or line number.
+  C << u8"\u200b";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(5U, C.getColumn());
+  EXPECT_EQ(15U, C.GetNumBytesInBuffer());
+}
+
+TEST(formatted_raw_ostreamTest, Test_UTF8Buffered) {
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(4);
+  formatted_raw_ostream C(B);
+
+  // This character encodes as three bytes, so will cause the buffer to be
+  // flushed after the first byte (4 byte buffer, 3 bytes already written). We
+  // need to save the first part of the UTF-8 encoding until after the buffer is
+  // cleared and the remaining two bytes are written, at which point we can
+  // check the display width. In this case the display width is 1, so we end at
+  // column 4, with 6 bytes written into total, 2 of which are in the buffer.
+  C << u8"123\u2468";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(4U, C.getColumn());
+  EXPECT_EQ(2U, C.GetNumBytesInBuffer());
+  C.flush();
+  EXPECT_EQ(6U, A.size());
+
+  // Same as above, but with a CJK character which displays as two columns.
+  C << u8"123\u55b5";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(9U, C.getColumn());
+  EXPECT_EQ(2U, C.GetNumBytesInBuffer());
+  C.flush();
+  EXPECT_EQ(12U, A.size());
+}
+
+TEST(formatted_raw_ostreamTest, Test_UTF8TinyBuffer) {
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(1);
+  formatted_raw_ostream C(B);
+
+  // The stream has a one-byte buffer, so it gets flushed multiple times while
+  // printing a single Unicode character.
+  C << u8"\u2468";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(1U, C.getColumn());
+  EXPECT_EQ(0U, C.GetNumBytesInBuffer());

[PATCH] D74619: [ARM] Enabling range checks on Neon intrinsics' lane arguments

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

I agree with @dnsampaio here, it's better to match the existing style, and 
avoid irrelevant churn in the git history.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74619



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

[PATCH] D76291: [Support] Fix formatted_raw_ostream for UTF-8

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard updated this revision to Diff 251030.
ostannard added a comment.

- I managed to reproduce the lsl-zero.s test failure on windows, this turned 
out to be because the test merges stdout and stderr, which can result in them 
being interleaved in ways which breaks tests. The test doesn't check anything 
in stderr, so fixed by redirecting that to /dev/null.
- The unit test was failing when built with MSVC, fixed by explicitly using 
UTF-8 string literals.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76291

Files:
  clang/test/Analysis/checker-plugins.c
  llvm/include/llvm/Support/FormattedStream.h
  llvm/lib/Support/FormattedStream.cpp
  llvm/test/MC/ARM/lsl-zero.s
  llvm/unittests/Support/formatted_raw_ostream_test.cpp

Index: llvm/unittests/Support/formatted_raw_ostream_test.cpp
===
--- llvm/unittests/Support/formatted_raw_ostream_test.cpp
+++ llvm/unittests/Support/formatted_raw_ostream_test.cpp
@@ -29,4 +29,143 @@
   }
 }
 
+TEST(formatted_raw_ostreamTest, Test_LineColumn) {
+  // Test tracking of line and column numbers in a stream.
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  formatted_raw_ostream C(B);
+
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  C << "a";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(1U, C.getColumn());
+
+  C << "bcdef";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(6U, C.getColumn());
+
+  // '\n' increments line number, sets column to zero.
+  C << "\n";
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  // '\r sets column to zero without changing line number
+  C << "foo\r";
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  // '\t' advances column to the next multiple of 8.
+  // FIXME: If the column number is already a multiple of 8 this will do
+  // nothing, is this behaviour correct?
+  C << "1\t";
+  EXPECT_EQ(8U, C.getColumn());
+  C << "\t";
+  EXPECT_EQ(8U, C.getColumn());
+  C << "1234567\t";
+  EXPECT_EQ(16U, C.getColumn());
+  EXPECT_EQ(1U, C.getLine());
+}
+
+TEST(formatted_raw_ostreamTest, Test_Flush) {
+  // Flushing the buffer causes the characters in the buffer to be scanned
+  // before the buffer is emptied, so line and column numbers will still be
+  // tracked properly.
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(32);
+  formatted_raw_ostream C(B);
+
+  C << "\nabc";
+  EXPECT_EQ(4U, C.GetNumBytesInBuffer());
+  C.flush();
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(3U, C.getColumn());
+  EXPECT_EQ(0U, C.GetNumBytesInBuffer());
+}
+
+TEST(formatted_raw_ostreamTest, Test_UTF8) {
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(32);
+  formatted_raw_ostream C(B);
+
+  // U+00A0 Non-breaking space: encoded as two bytes, but only one column wide.
+  C << u8"\u00a0";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(1U, C.getColumn());
+  EXPECT_EQ(2U, C.GetNumBytesInBuffer());
+
+  // U+2468 CIRCLED DIGIT NINE: encoded as three bytes, but only one column
+  // wide.
+  C << u8"\u2468";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(2U, C.getColumn());
+  EXPECT_EQ(5U, C.GetNumBytesInBuffer());
+
+  // U+0001 LINEAR B SYLLABLE B008 A: encoded as four bytes, but only one
+  // column wide.
+  C << u8"\U0001";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(3U, C.getColumn());
+  EXPECT_EQ(9U, C.GetNumBytesInBuffer());
+
+  // U+55B5, CJK character, encodes as three bytes, takes up two columns.
+  C << u8"\u55b5";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(5U, C.getColumn());
+  EXPECT_EQ(12U, C.GetNumBytesInBuffer());
+
+  // U+200B, zero-width space, encoded as three bytes but has no effect on the
+  // column or line number.
+  C << u8"\u200b";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(5U, C.getColumn());
+  EXPECT_EQ(15U, C.GetNumBytesInBuffer());
+}
+
+TEST(formatted_raw_ostreamTest, Test_UTF8Buffered) {
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(4);
+  formatted_raw_ostream C(B);
+
+  // This character encodes as three bytes, so will cause the buffer to be
+  // flushed after the first byte (4 byte buffer, 3 bytes already written). We
+  // need to save the first part of the UTF-8 encoding until after the buffer is
+  // cleared and the remaining two bytes are written, at which point we can
+  // check the display width. In this case the display width is 1, so we end at
+  // column 4, with 6 bytes written into total, 2 of which are in the buffer.
+  C << u8"123\u2468";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(4U, C.getColumn());
+  EXPECT_EQ(2U, C.GetNumBytesInBuffer());
+  C.flush();
+  EXPECT_EQ(6U, A.size());
+
+  // Same as above, but with a CJK character which displays as two columns.
+  C << u8"123\u55b5";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(9U, C.getColumn());
+  EXPECT_EQ(2U, C.GetNumBytesInBuffer());
+  C.flush();
+  EXPECT_EQ(12U, A.size());

[PATCH] D76291: [Support] Fix formatted_raw_ostream for UTF-8

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard marked 16 inline comments as done.
ostannard added inline comments.



Comment at: llvm/unittests/Support/formatted_raw_ostream_test.cpp:88
+
+TEST(formatted_raw_ostreamTest, Test_UTF8) {
+  SmallString<128> A;

hubert.reinterpretcast wrote:
> Should there be a test for combining characters?
This doesn't support combining characters, I don't think there's much point in 
adding a test for something which doesn't work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76291



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

[PATCH] D76291: [Support] Fix formatted_raw_ostream for UTF-8

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard created this revision.
ostannard added reviewers: MaskRay, thakis, hoyFB, benlangmuir.
Herald added subscribers: cfe-commits, dexonsmith, hiraditya.
Herald added a project: clang.

- The getLine and getColumn functions need to update the position, or they will 
return stale data for buffered streams. This fixes a bug in the clang 
-analyzer-checker-option-help option, which was not wrapping the help text 
correctly when stdout is not a TTY.
- If the stream contains multi-byte UTF-8 sequences, then the whole sequence 
needs to be considered to be a single character. This has the edge case that 
the buffer might fill up and be flushed part way through a character.
- If the stream contains East Asian wide characters, these will be rendered 
twice as wide as other characters, so we need to increase the column count to 
match.

This doesn't attempt to handle everything unicode can do (combining characters, 
right-to-left markers, ...), but hopefully covers most things likely to be 
common in messages and source code we might want to print.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76291

Files:
  clang/test/Analysis/checker-plugins.c
  llvm/include/llvm/Support/FormattedStream.h
  llvm/lib/Support/FormattedStream.cpp
  llvm/unittests/Support/formatted_raw_ostream_test.cpp

Index: llvm/unittests/Support/formatted_raw_ostream_test.cpp
===
--- llvm/unittests/Support/formatted_raw_ostream_test.cpp
+++ llvm/unittests/Support/formatted_raw_ostream_test.cpp
@@ -29,4 +29,144 @@
   }
 }
 
+TEST(formatted_raw_ostreamTest, Test_LineColumn) {
+  // Test tracking of line and column numbers in a stream.
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  formatted_raw_ostream C(B);
+
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  C << "a";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(1U, C.getColumn());
+
+  C << "bcdef";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(6U, C.getColumn());
+
+  // '\n' increments line number, sets column to zero.
+  C << "\n";
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  // '\r sets coulmn to zero without changing line number
+  C << "foo\r";
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(0U, C.getColumn());
+
+  // '\t' advances column to the next multiple of 8.
+  // FIXME: If the column number is already a multiple of 8 this will do
+  // nothing, is this behaviour correct?
+  C << "1\t";
+  EXPECT_EQ(8U, C.getColumn());
+  C << "\t";
+  EXPECT_EQ(8U, C.getColumn());
+  C << "1234567\t";
+  EXPECT_EQ(16U, C.getColumn());
+  EXPECT_EQ(1U, C.getLine());
+}
+
+TEST(formatted_raw_ostreamTest, Test_Flush) {
+  // Flushing the buffer causes the charcters in the buffer to be scanned
+  // before the buffer is emptied, so line and column numbers will still be
+  // tracked properly.
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(32);
+  formatted_raw_ostream C(B);
+
+  C << "\nabc";
+  EXPECT_EQ(4U, C.GetNumBytesInBuffer());
+  C.flush();
+  EXPECT_EQ(1U, C.getLine());
+  EXPECT_EQ(3U, C.getColumn());
+  EXPECT_EQ(0U, C.GetNumBytesInBuffer());
+}
+
+TEST(formatted_raw_ostreamTest, Test_UTF8) {
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(32);
+  formatted_raw_ostream C(B);
+
+  // U+00A0 Non-breaking space: encoded as two bytes, but only one column wide.
+  C << "\u00a0";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(1U, C.getColumn());
+  EXPECT_EQ(2U, C.GetNumBytesInBuffer());
+
+  // U+2468 CIRCLED DIGIT NINE: encoded as three bytes, but only one column
+  // wide.
+  C << "\u2468";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(2U, C.getColumn());
+  EXPECT_EQ(5U, C.GetNumBytesInBuffer());
+
+  // U+0001 LINEAR B SYLLABLE B008 A: encoded as four bytes, but only one
+  // column wide.
+  C << "\U0001";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(3U, C.getColumn());
+  EXPECT_EQ(9U, C.GetNumBytesInBuffer());
+
+  // U+55B5, chinese character, encodes as three bytes, takes up two columns.
+  C << "\u55b5";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(5U, C.getColumn());
+  EXPECT_EQ(12U, C.GetNumBytesInBuffer());
+
+  // U+200B, zero-width space, encoded as three bytes but has no effect on the
+  // column or line number.
+  C << "\u200b";
+  EXPECT_EQ(0U, C.getLine());
+  EXPECT_EQ(5U, C.getColumn());
+  EXPECT_EQ(15U, C.GetNumBytesInBuffer());
+}
+
+TEST(formatted_raw_ostreamTest, Test_UTF8Buffered) {
+  SmallString<128> A;
+  raw_svector_ostream B(A);
+  B.SetBufferSize(4);
+  formatted_raw_ostream C(B);
+
+  // This character encodes as three bytes, so will cause the buffer to be
+  // flushed after the first byte (4 byte buffer, 3 bytes already written). We
+  // need to save the first part of the UTF-8 encoding until after the buffer is
+  // cleared and the remaining two bytes are written, at which point we can
+  // check the display width. In this case the display width is 1, so we end at
+  // colu

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

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

I've not looked at the code in detail yet, but I think this still gets the ABI 
wrong for this example:

  typedef struct {
__attribute__ ((__aligned__(32))) double v[4];
  } TYPE1;
  
  double func(double d0, double d1, double d2, double d3,
  double d4, double d5, double d6, double d7,
  float stk0, TYPE1 stk1) {
return stk1.v[0];
  }

The ABI says 
(https://github.com/ARM-software/abi-aa/blob/master/aapcs64/aapcs64.rst, rule 
B.5):

  If the argument is an alignment adjusted type its value is passed as a copy 
of the actual value. The copy will have an alignment defined as follows.
  * For a Fundamental Data Type, the alignment is the natural alignment of that 
type, after any promotions.
  * For a Composite Type, the alignment of the copy will have 8-byte alignment 
if its natural alignment is <= 8 and 16-byte alignment if its natural alignment 
is >= 16.
  The alignment of the copy is used for applying marshaling rules.

This means that `stk1` should be passed as a copy with alignment 16 bytes, 
putting it at `sp+16`. GCC does this, clang without this patch passes it at 
`sp+8`, and clang with this patch passes it at `sp+32`.


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] D73534: [DebugInfo] Enable the debug entry values feature by default

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

Reproducer for that crash: P8198  P8199 



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

https://reviews.llvm.org/D73534



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

[PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

There's a failure on an ARM 2-stage buildbot which looks related to this: 
http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-full-sh/builds/3920/steps/build%20stage%202/logs/stdio

I can reproduce the crash on the buildbot machine, so hopefully I'll have a 
standalone reproducer for you soon.


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

https://reviews.llvm.org/D73534



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

[PATCH] D72932: [ARM] Follow AACPS standard for volatile bit-fields access width

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

I think I've spotted a bug in the ABI spec, which you've faithfully implemented 
here. I don't know of any other compiler which has implemented this ABI change 
yet, so it's probably worth seeing if we can get the spec fixed.

The intention of the ABI is to avoid conflicting with the C/C++11 spec, which 
requires access to one "memory location" to not write to any adjacent memory 
locations. However, the wording of the ABI does not take into account 
zero-sized bitfields, which are defined in C/C++ to start a new memory 
location. For example:

  struct foo {
int a : 8;
char  : 0;
int b : 8;
  };

Here, C/C++ says that `a` and `b` are separate memory locations, so it must be 
possible to read and write them from two threads. However, the ABI does not 
have the special case for zero-sized bitfields, so requires access to both 
fields to use 32-bit loads and stores, which overlap.

I've raised a ticket (internal to Arm) to consider changing the ABI to match 
C/C++ in this case.




Comment at: clang/include/clang/Basic/CodeGenOptions.def:396
+/// according to the field declaring type width.
+CODEGENOPT(ForceNoAAPCSBitfieldWidth, 1, 0)
+

Why is this a negative option, when the one above is positive?



Comment at: clang/include/clang/Driver/Options.td:2328
   HelpText<"Follows the AAPCS standard that all volatile bit-field write 
generates at least one load. (ARM only).">;
+def ForceNoAAPCSBitfieldWidth : Flag<["-"], "fno-AAPCSBitfieldWidth">, 
Group,
+  Flags<[DriverOption,CC1Option]>,

Command-line options are in kebab-case, so this should be something like 
`fno-aapcs-bitfield-width`. This also applies to the `fAAPCSBitfieldLoad` 
option above, assuming it's not too late to change that.

Please also add a positive version of this option (i.e. 
`faapcs-bitfield-width`).



Comment at: clang/include/clang/Driver/Options.td:2330
+  Flags<[DriverOption,CC1Option]>,
+  HelpText<"Does not follow the AAPCS standard that volatile bit-field width 
is dictated by the field declarative type. (ARM only).">;
 

s/Does/Do/

s/declarative/container/



Comment at: clang/lib/CodeGen/CGRecordLayout.h:100
+ unsigned StorageSize, CharUnits StorageOffset,
+ unsigned VolatileOffset, unsigned VolatileStorageSize,
+ CharUnits VolatileStorageOffset)

It doesn't look like this is ever called with different volatile/non-volatile 
values, so I don't think we need the extra parameters.



Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:525
+/// When a volatile bit-field is read, and its container does not overlap
+/// with any non-bit-field member, itscontainer must be read exactly once
+/// using the access width appropriate to the type of the container.

Space in "itscontainer".



Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:528
+/// When a volatile bit-field is written, and its container does not overlap
+/// with any non-bit-field member, itscontainer must be read exactly once and
+/// written exactly once using the access width appropriate to thetype of the

Space in "itscontainer".



Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:529
+/// with any non-bit-field member, itscontainer must be read exactly once and
+/// written exactly once using the access width appropriate to thetype of the
+/// container. The two accesses are not atomic.

Space in "thetype".



Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:541
+llvm::Type *ResLTy = Types.ConvertTypeForMem(Field->getType());
+// If the register alignment is less than the type width, we can't enforce 
a
+// aligned load, bail out

_register_ alignment?



Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:542
+// If the register alignment is less than the type width, we can't enforce 
a
+// aligned load, bail out
+if ((uint64_t)(Context.toBits(Layout.getAlignment())) <

Comments end in a full stop (multiple times in this function).



Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:631
   if (Layout.hasOwnVFPtr())
-Members.push_back(MemberInfo(CharUnits::Zero(), MemberInfo::VFPtr,
-llvm::FunctionType::get(getIntNType(32), /*isVarArg=*/true)->
-getPointerTo()->getPointerTo()));
+Members.push_back(
+MemberInfo(CharUnits::Zero(), MemberInfo::VFPtr,

Unnecessary formatting change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72932



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

[PATCH] D72932: [ARM] Follow AACPS standard for volatile bit-fields access width

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

> It will require changing all possible initializations, with a sensible value.

Where are you seeing multiple initializations? It looks like all of the logic 
for struct layout happens in `CGRecordLowering::lower`, we might need to add a 
pass there to calculate the valid access widths for bitfields.

> And either way, codegen will also need to change, to define the address, it 
> must know if it is volatile or not.

The difference is that codegen only needs to change to check if the access is 
volatile, and select the correct offset and size already calculated during 
struct layout.

> perhaps we can leave it as a TODO to move it there?

I'm less concerned about the performance impact of this, and more about making 
this code harder to understand by spreading the logic across multiple different 
phases of compilation. This has been broken for years, I'd rather we fix it 
properly, than add a TODO which will never get done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72932



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

[PATCH] D72932: [ARM] Follow AACPS standard for volatile bit-fields access width

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

Why are you doing this in CodeGen, rather than adjusting the existing layout 
code in CGRecordLowering? Doing it this way will result in 
AdjustAAPCSBitfieldLValue being called for every access to the bitfield, rather 
than just once. This is probably more fragile too, because it's spreading the 
logic across multiple parts of the codebase, and has to undo some of the layout 
done by CGRecordLowering.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72932



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

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard accepted this revision.
ostannard 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/D7/new/

https://reviews.llvm.org/D7



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

[PATCH] D72221: Support function attribute patchable_function_entry

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:3995
+``__attribute__((patchable_function_entry(N,M)))`` is used to generate M NOPs
+before the function entry and N-M NOPs after the function entry. This 
attributes
+takes precedence over command line option ``-fpatchable-function-entry=N,M``.

Grammar nits:
s/attributes/attribute/
over _the_ command line option



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:826
+  "patchable-function-entry",
+  (Twine(Attr->getSize()) + "," + Twine(Attr->getStart())).str());
   }

I think using two function attributes here would be better, to avoid needing to 
parse this again later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72221



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

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,M]

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:77
   "invalid unwind library name in argument '%0'">;
+def err_drv_incompatible_options : Error<"'%0' and '%1' cannot be used 
together">;
 def err_drv_incompatible_unwindlib : Error<

We already have `err_drv_argument_not_allowed_with`, which can be used instead 
of adding this.



Comment at: clang/include/clang/Driver/Options.td:1703
   HelpText<"Recognize and construct Pascal-style string literals">;
+def fpatchable_function_entry : Joined<["-"], "fpatchable-function-entry=">, 
Group, Flags<[CC1Option]>,
+  HelpText<"Generate N NOPs at function entry">;

Names of options with an equals sign end in `_EQ`.



Comment at: clang/test/Driver/fpatchable-function-entry.c:11
+// RUN: not %clang -target i386 -fsyntax-only %s 
-fpatchable-function-entry=1,1 2>&1 | FileCheck --check-prefix=NONZERO %s
+// NONZERO: error: the clang compiler does not support '1'
+

I think we should have a more descriptive message here, maybe something like:

  error: the second parameter of '-fpatchable-function-entry=' must be '0' or 
omitted


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D7



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

[PATCH] D67216: [cfi] Add flag to always generate .debug_frame

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D67216



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

[PATCH] D67216: [cfi] Add flag to always generate .debug_frame

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

How about `-f[no-]force-dwarf-frame`, which I think matches the spelling and 
behaviour of the existing `-fforce-enable-int128` and `-fforce-emit-vtables` 
options?


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

https://reviews.llvm.org/D67216



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

[PATCH] D67216: [cfi] Add flag to always generate .debug_frame

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

I don't like the idea of adding a `-gno-dwarf-frame` option which doesn't 
always disable emission of `.debug_frame`. Since it doesn't make sense to emit 
other debug info without `.debug_frame`, maybe we should just not have a 
negative option?


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

https://reviews.llvm.org/D67216



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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9f6a873268e1: Dead Virtual Function Elimination (authored by 
ostannard).

Changed prior to commit:
  https://reviews.llvm.org/D63932?vs=218363&id=224568#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGenCXX/vcall-visibility-metadata.cpp
  clang/test/CodeGenCXX/virtual-function-elimination.cpp
  clang/test/Driver/virtual-function-elimination.cpp
  llvm/docs/LangRef.rst
  llvm/docs/TypeMetadata.rst
  llvm/include/llvm/Analysis/TypeMetadataUtils.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/GlobalObject.h
  llvm/include/llvm/Transforms/IPO/GlobalDCE.h
  llvm/lib/Analysis/TypeMetadataUtils.cpp
  llvm/lib/IR/Metadata.cpp
  llvm/lib/LTO/LTO.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/lib/Transforms/IPO/GlobalDCE.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/test/LTO/ARM/lto-linking-metadata.ll
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-base-call.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-base-pointer-call.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-derived-call.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-derived-pointer-call.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-visibility-post-lto.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-visibility-pre-lto.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions.ll
  llvm/test/Transforms/GlobalDCE/vtable-rtti.ll
  llvm/test/Transforms/Internalize/vcall-visibility.ll

Index: llvm/test/Transforms/Internalize/vcall-visibility.ll
===
--- /dev/null
+++ llvm/test/Transforms/Internalize/vcall-visibility.ll
@@ -0,0 +1,64 @@
+; RUN: opt < %s -internalize -S | FileCheck %s
+
+%struct.A = type { i32 (...)** }
+%struct.B = type { i32 (...)** }
+%struct.C = type { i32 (...)** }
+
+; Class A has default visibility, so has no !vcall_visibility metadata before
+; or after LTO.
+; CHECK-NOT: @_ZTV1A = {{.*}}!vcall_visibility
+@_ZTV1A = dso_local unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* bitcast (void (%struct.A*)* @_ZN1A3fooEv to i8*)] }, align 8, !type !0, !type !1
+
+; Class B has hidden visibility but public LTO visibility, so has no
+; !vcall_visibility metadata before or after LTO.
+; CHECK-NOT: @_ZTV1B = {{.*}}!vcall_visibility
+@_ZTV1B = hidden unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* bitcast (void (%struct.B*)* @_ZN1B3fooEv to i8*)] }, align 8, !type !2, !type !3
+
+; Class C has hidden visibility, so the !vcall_visibility metadata is set to 1
+; (linkage unit) before LTO, and 2 (translation unit) after LTO.
+; CHECK: @_ZTV1C ={{.*}}!vcall_visibility [[MD_TU_VIS:![0-9]+]]
+@_ZTV1C = hidden unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* bitcast (void (%struct.C*)* @_ZN1C3fooEv to i8*)] }, align 8, !type !4, !type !5, !vcall_visibility !6
+
+; Class D has translation unit visibility before LTO, and this is not changed
+; by LTO.
+; CHECK: @_ZTVN12_GLOBAL__N_11DE = {{.*}}!vcall_visibility [[MD_TU_VIS:![0-9]+]]
+@_ZTVN12_GLOBAL__N_11DE = internal unnamed_addr constant { [3 x i8*] } zeroinitializer, align 8, !type !7, !type !9, !vcall_visibility !11
+
+define dso_local void @_ZN1A3fooEv(%struct.A* nocapture %this) {
+entry:
+  ret void
+}
+
+define hidden void @_ZN1B3fooEv(%struct.B* nocapture %this) {
+entry:
+  ret void
+}
+
+define hidden void @_ZN1C3fooEv(%struct.C* nocapture %this) {
+entry:
+  ret void
+}
+
+define hidden noalias nonnull i8* @_Z6make_dv() {
+entry:
+  %call = tail call i8* @_Znwm(i64 8) #3
+  %0 = bitcast i8* %call to i32 (...)***
+  store i32 (...)** bitcast (i8** getelementptr inbounds ({ [3 x i8*] }, { [3 x i8*] }* @_ZTVN12_GLOBAL__N_11DE, i64 0, inrange i32 0, i64 2) to i32 (...)**), i32 (...)*** %0, align 8
+  ret i8* %call
+}
+
+declare dso_local noalias nonnull i8* @_Znwm(i64)
+
+; CHECK: [[MD_TU_VIS]] = !{i64 2}
+!0 = !{i64 16, !"_ZTS1A"}
+!1 = !{i64 16, !"_ZTSM1AFvvE.virtual"}
+!2 = !{i64 16, !"_ZTS1B"}
+!3 = !{i64 16, !"_ZTSM1BFvvE.virtual"}
+!4 = !{i64 16, !"_ZTS1C"}
+!5 = !{i64 16, !"_ZTSM1CFvvE.virtual"}
+!6 = !{i64 1}
+!7 = !{i64 16, !8}
+!8 = distinct !{}
+!9 = !{i64 16, !10}
+!10 = distinct !{}
+!11 = !{i64 2}
Index: llvm/test/Transforms/GlobalDCE/vtable-rtti.ll
===
--- /dev/null
+++ llvm/test/Transforms/GlobalDCE/vtable-rtti.ll
@@ -0,0 +1,47 @@
+; RUN: opt < %s -globaldce -S 

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932



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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932



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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

> This makes the IR self-contained, which is good, but it also make the 
> interpretation of the IR modal, which isn't great. It means that suddenly the 
> rules of interpretation of what is valid to do or not changes according to 
> this module flag.

I think the original version was better from that perspective - most compiler 
passes only need to check for one value of the attribute, and only the linker 
needed to care about the difference between link-unit and public visibility. Do 
you think we should go back to that design, or do you have a different idea?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932



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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932



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

[PATCH] D67467: [ARM] Update clang for removal of vfp2d16 and vfp2d16sp

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard accepted this revision.
ostannard added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: dmgreen.

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D67467



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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932



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

[PATCH] D67160: [clang, ARM] Default to -fno-lax-vector-conversions in ARM v8.1-M.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard accepted this revision.
ostannard 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/D67160/new/

https://reviews.llvm.org/D67160



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

[PATCH] D67216: [cfi] Add flag to always generate call frame information

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

Does the name of the `-falways-need-cfi` option match any existing compiler 
(google doesn't find anything)? If not, I think it would make more sense as a 
`-g` option, as it's similar to `-gline-tables-only`.




Comment at: clang/lib/Frontend/CompilerInvocation.cpp:956
  OPT_fno_unique_section_names, true);
+  Opts.AlwaysNeedCFI =
+  Args.hasFlag(OPT_falways_need_cfi, OPT_fno_always_need_cfi, false);

Is this option actually being read anywhere?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67216



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

[PATCH] D67160: [clang, ARM] Default to -fno-lax-vector-conversions in ARM v8.1-M.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

Test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67160



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

[PATCH] D65863: [ARM] Add support for the s,j,x,N,O inline asm constraints

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

LGTM with one small change.




Comment at: clang/test/Sema/arm_inline_asm_constraints.c:47
+// I: An immediate integer valid for a data-processing instruction. 
(ARM/Thumb2)
+//An immediate integer between -255 and -1. (Thumb1)
+int test_I(int i) {

Copy-paste error, should be "between 0 and 255".


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

https://reviews.llvm.org/D65863



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

[PATCH] D66588: [ARM NEON] Avoid duplicated decarations

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

LGTM with one small nit.




Comment at: clang/utils/TableGen/NeonEmitter.cpp:1906
 std::string Intrinsic::generate() {
+  // Avoid duplicated code for big and small endians
+  if (isBigEndianSafe()) {

s/small endians/little endian/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66588



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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard marked 4 inline comments as done.
ostannard added a comment.

> It isn't that common, but it seems worth doing if it can be done easily.



> That said, I note that it does appear that your implementation will end up 
> preserving the pointer in the vtable in this case because you're relying on 
> the use list to make decisions about what to GC. So it doesn't seem easy to 
> do at this point, but if for example we made this compatible with ThinLTO at 
> some point we would probably not be able to rely on the use list, and the 
> resulting changes to this feature might make this easier to do.

Ok, I think that it makes sense to leave this for a separate patch, as long as 
we currently generate correct code. I've added partial linking of the LTO unit 
to my fuzzer, and haven't found any problems.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932



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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard updated this revision to Diff 218363.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGenCXX/vcall-visibility-metadata.cpp
  clang/test/CodeGenCXX/virtual-function-elimination.cpp
  clang/test/Driver/virtual-function-elimination.cpp
  llvm/docs/LangRef.rst
  llvm/docs/TypeMetadata.rst
  llvm/include/llvm/Analysis/TypeMetadataUtils.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/GlobalObject.h
  llvm/include/llvm/Transforms/IPO/GlobalDCE.h
  llvm/lib/Analysis/TypeMetadataUtils.cpp
  llvm/lib/IR/Metadata.cpp
  llvm/lib/LTO/LTO.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/lib/Transforms/IPO/GlobalDCE.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/test/LTO/ARM/lto-linking-metadata.ll
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-base-call.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-base-pointer-call.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-derived-call.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-derived-pointer-call.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-visibility-post-lto.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-visibility-pre-lto.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions.ll
  llvm/test/Transforms/GlobalDCE/vtable-rtti.ll
  llvm/test/Transforms/Internalize/vcall-visibility.ll

Index: llvm/test/Transforms/Internalize/vcall-visibility.ll
===
--- /dev/null
+++ llvm/test/Transforms/Internalize/vcall-visibility.ll
@@ -0,0 +1,64 @@
+; RUN: opt < %s -internalize -S | FileCheck %s
+
+%struct.A = type { i32 (...)** }
+%struct.B = type { i32 (...)** }
+%struct.C = type { i32 (...)** }
+
+; Class A has default visibility, so has no !vcall_visibility metadata before
+; or after LTO.
+; CHECK-NOT: @_ZTV1A = {{.*}}!vcall_visibility
+@_ZTV1A = dso_local unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* bitcast (void (%struct.A*)* @_ZN1A3fooEv to i8*)] }, align 8, !type !0, !type !1
+
+; Class B has hidden visibility but public LTO visibility, so has no
+; !vcall_visibility metadata before or after LTO.
+; CHECK-NOT: @_ZTV1B = {{.*}}!vcall_visibility
+@_ZTV1B = hidden unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* bitcast (void (%struct.B*)* @_ZN1B3fooEv to i8*)] }, align 8, !type !2, !type !3
+
+; Class C has hidden visibility, so the !vcall_visibility metadata is set to 1
+; (linkage unit) before LTO, and 2 (translation unit) after LTO.
+; CHECK: @_ZTV1C ={{.*}}!vcall_visibility [[MD_TU_VIS:![0-9]+]]
+@_ZTV1C = hidden unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* bitcast (void (%struct.C*)* @_ZN1C3fooEv to i8*)] }, align 8, !type !4, !type !5, !vcall_visibility !6
+
+; Class D has translation unit visibility before LTO, and this is not changed
+; by LTO.
+; CHECK: @_ZTVN12_GLOBAL__N_11DE = {{.*}}!vcall_visibility [[MD_TU_VIS:![0-9]+]]
+@_ZTVN12_GLOBAL__N_11DE = internal unnamed_addr constant { [3 x i8*] } zeroinitializer, align 8, !type !7, !type !9, !vcall_visibility !11
+
+define dso_local void @_ZN1A3fooEv(%struct.A* nocapture %this) {
+entry:
+  ret void
+}
+
+define hidden void @_ZN1B3fooEv(%struct.B* nocapture %this) {
+entry:
+  ret void
+}
+
+define hidden void @_ZN1C3fooEv(%struct.C* nocapture %this) {
+entry:
+  ret void
+}
+
+define hidden noalias nonnull i8* @_Z6make_dv() {
+entry:
+  %call = tail call i8* @_Znwm(i64 8) #3
+  %0 = bitcast i8* %call to i32 (...)***
+  store i32 (...)** bitcast (i8** getelementptr inbounds ({ [3 x i8*] }, { [3 x i8*] }* @_ZTVN12_GLOBAL__N_11DE, i64 0, inrange i32 0, i64 2) to i32 (...)**), i32 (...)*** %0, align 8
+  ret i8* %call
+}
+
+declare dso_local noalias nonnull i8* @_Znwm(i64)
+
+; CHECK: [[MD_TU_VIS]] = !{i64 2}
+!0 = !{i64 16, !"_ZTS1A"}
+!1 = !{i64 16, !"_ZTSM1AFvvE.virtual"}
+!2 = !{i64 16, !"_ZTS1B"}
+!3 = !{i64 16, !"_ZTSM1BFvvE.virtual"}
+!4 = !{i64 16, !"_ZTS1C"}
+!5 = !{i64 16, !"_ZTSM1CFvvE.virtual"}
+!6 = !{i64 1}
+!7 = !{i64 16, !8}
+!8 = distinct !{}
+!9 = !{i64 16, !10}
+!10 = distinct !{}
+!11 = !{i64 2}
Index: llvm/test/Transforms/GlobalDCE/vtable-rtti.ll
===
--- /dev/null
+++ llvm/test/Transforms/GlobalDCE/vtable-rtti.ll
@@ -0,0 +1,47 @@
+; RUN: opt < %s -globaldce -S | FileCheck %s
+
+; We currently only use llvm.type.checked.load for virtual function pointers,
+; not any other part of the vtable, so we can't remove the RTTI pointer even if
+; it's never going to be 

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard updated this revision to Diff 212833.
ostannard added a comment.

- Add LTOPostLink metadata, instead of internalising vcall visibility at LTO 
time


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGenCXX/vcall-visibility-metadata.cpp
  clang/test/CodeGenCXX/virtual-function-elimination.cpp
  clang/test/Driver/virtual-function-elimination.cpp
  llvm/docs/LangRef.rst
  llvm/docs/TypeMetadata.rst
  llvm/include/llvm/Analysis/TypeMetadataUtils.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/GlobalObject.h
  llvm/include/llvm/Transforms/IPO/GlobalDCE.h
  llvm/lib/Analysis/TypeMetadataUtils.cpp
  llvm/lib/IR/Metadata.cpp
  llvm/lib/LTO/LTO.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/lib/Transforms/IPO/GlobalDCE.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/test/LTO/ARM/lto-linking-metadata.ll
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-base-call.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-base-pointer-call.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-derived-call.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-derived-pointer-call.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-visibility-post-lto.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-visibility-pre-lto.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions.ll
  llvm/test/Transforms/Internalize/vcall-visibility.ll

Index: llvm/test/Transforms/Internalize/vcall-visibility.ll
===
--- /dev/null
+++ llvm/test/Transforms/Internalize/vcall-visibility.ll
@@ -0,0 +1,64 @@
+; RUN: opt < %s -internalize -S | FileCheck %s
+
+%struct.A = type { i32 (...)** }
+%struct.B = type { i32 (...)** }
+%struct.C = type { i32 (...)** }
+
+; Class A has default visibility, so has no !vcall_visibility metadata before
+; or after LTO.
+; CHECK-NOT: @_ZTV1A = {{.*}}!vcall_visibility
+@_ZTV1A = dso_local unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* bitcast (void (%struct.A*)* @_ZN1A3fooEv to i8*)] }, align 8, !type !0, !type !1
+
+; Class B has hidden visibility but public LTO visibility, so has no
+; !vcall_visibility metadata before or after LTO.
+; CHECK-NOT: @_ZTV1B = {{.*}}!vcall_visibility
+@_ZTV1B = hidden unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* bitcast (void (%struct.B*)* @_ZN1B3fooEv to i8*)] }, align 8, !type !2, !type !3
+
+; Class C has hidden visibility, so the !vcall_visibility metadata is set to 1
+; (linkage unit) before LTO, and 2 (translation unit) after LTO.
+; CHECK: @_ZTV1C ={{.*}}!vcall_visibility [[MD_TU_VIS:![0-9]+]]
+@_ZTV1C = hidden unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* bitcast (void (%struct.C*)* @_ZN1C3fooEv to i8*)] }, align 8, !type !4, !type !5, !vcall_visibility !6
+
+; Class D has translation unit visibility before LTO, and this is not changed
+; by LTO.
+; CHECK: @_ZTVN12_GLOBAL__N_11DE = {{.*}}!vcall_visibility [[MD_TU_VIS:![0-9]+]]
+@_ZTVN12_GLOBAL__N_11DE = internal unnamed_addr constant { [3 x i8*] } zeroinitializer, align 8, !type !7, !type !9, !vcall_visibility !11
+
+define dso_local void @_ZN1A3fooEv(%struct.A* nocapture %this) {
+entry:
+  ret void
+}
+
+define hidden void @_ZN1B3fooEv(%struct.B* nocapture %this) {
+entry:
+  ret void
+}
+
+define hidden void @_ZN1C3fooEv(%struct.C* nocapture %this) {
+entry:
+  ret void
+}
+
+define hidden noalias nonnull i8* @_Z6make_dv() {
+entry:
+  %call = tail call i8* @_Znwm(i64 8) #3
+  %0 = bitcast i8* %call to i32 (...)***
+  store i32 (...)** bitcast (i8** getelementptr inbounds ({ [3 x i8*] }, { [3 x i8*] }* @_ZTVN12_GLOBAL__N_11DE, i64 0, inrange i32 0, i64 2) to i32 (...)**), i32 (...)*** %0, align 8
+  ret i8* %call
+}
+
+declare dso_local noalias nonnull i8* @_Znwm(i64)
+
+; CHECK: [[MD_TU_VIS]] = !{i64 2}
+!0 = !{i64 16, !"_ZTS1A"}
+!1 = !{i64 16, !"_ZTSM1AFvvE.virtual"}
+!2 = !{i64 16, !"_ZTS1B"}
+!3 = !{i64 16, !"_ZTSM1BFvvE.virtual"}
+!4 = !{i64 16, !"_ZTS1C"}
+!5 = !{i64 16, !"_ZTSM1CFvvE.virtual"}
+!6 = !{i64 1}
+!7 = !{i64 16, !8}
+!8 = distinct !{}
+!9 = !{i64 16, !10}
+!10 = distinct !{}
+!11 = !{i64 2}
Index: llvm/test/Transforms/GlobalDCE/virtual-functions.ll
===
--- /dev/null
+++ llvm/test/Transforms/GlobalDCE/virtual-functions.ll
@@ -0,0 +1,55 @@
+; RUN: opt < %s -globaldce -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+declare dso_local noalias nonnull i8* @_Znwm(i

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

> Partial linking will indeed prevent dropping the virtual functions, but it 
> should not prevent clearing the pointer to the virtual function in the 
> vtable. The linker should then be able to drop the virtual function body as 
> part of `--gc-sections` during the final link.

If partial linking isn't doing internalisation, I'd expect that to prevent a 
lot of other LTO optimisations, not just VFE. Is this a common use-case which 
we need to care about?

> I think I would favour something closer to your first suggestion, but instead 
> of telling GlobalDCE specifically about this, we represent the "post-link" 
> flag in the IR (e.g. as a module flag) in order to keep the IR 
> self-contained. LTO would then be taught to add this flag at the right time, 
> and the logic inside GlobalDCE would be:
> 
> - If post-link flag not set, allow VFE if linkage <= TranslationUnit.
> - If post-link flag set, allow VFE if linkage <= LinkageUnit.
> 
>   This would also help address a correctness issue with the CFI and WPD 
> passes, which is that it is currently unsound to run them at compile time. If 
> we let them run at compile time, we would in principle be able to do CFI and 
> WPD on internal linkage types without LTO.

Ok, sounds reasonable, though I suspect WPD and CFI will need a slightly 
different definition of type visibility - they care about the possibility of 
unseen code adding new derived classes, so the visibility of base classes isn't 
important, and they might be able to make use of the final specifier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932



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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

> - Take the example from my earlier message, give the "main executable" and 
> "DSO" hidden visibility, build the "main executable" with LTO and the "DSO" 
> without LTO, and statically link them both into the same executable. We run 
> into the same problem where the Plugin1 vtable is potentially not referenced 
> and thus misoptimised. Yes, this is a violation of the LTO visibility rules, 
> but the example shows that we only detect it sometimes. I think that if we 
> did want to detect cases where the LTO visibility rules are clearly being 
> violated, the outcome should be to issue a diagnostic and not to silently 
> proceed with optimizations disabled, since the violation might be masking 
> other undetected issues. That really seems orthogonal to this patch, though.

As you say, this is a violation of the LTO visibility rules, so 
`[[clang::lto_visibility_public]]` must be used. I've gated this optimisation 
behind `-fwhole-program-vtables`, which if I've understood 
https://clang.llvm.org/docs/LTOVisibility.html correctly, is effectively the 
user asserting to the compiler that they've followed the LTO visility rules 
correctly.

> - Imagine linking part of a program with ld -r with LTO -- all symbols 
> including vtable symbols will appear to be externally visible, which is not 
> necessarily a violation of the LTO visibility rules because they may not be 
> used externally in the final link. So we end up pessimising unnecessarily.

I'm not really familiar with partial linking, is there any good documentation 
on how it is supposed to interact with LTO?

I've tried to make the changed to vcall_visibility as similar to the normal LTO 
internalisation process as possible, it happens at the same time and under the 
same conditions. If partial linking prevents normal internalisation, which will 
in turn prevent most of the existing optimisations done by GlobalDCE, then 
surely it should also prevent internalisation of vcall_visibility?

> I gave this some more thought and it seems that the frontend has all of the 
> information required to make a determination about whether to allow VFE, 
> without needing to teach LTO to relax visibility. We can make the frontend do 
> this:
> 
> - If -flto was passed (see CodeGenOptions::PrepareForLTO), attach "allow VFE" 
> metadata if class has hidden LTO visibility.
> - Otherwise, attach "allow VFE" metadata if class is not isExternallyVisible.
> 
>   Now the behaviour of GlobalDCE can be made conditional on the "allow VFE" 
> metadata alone. This also has the advantage of simplifying the IR by making 
> "allow VFE" a boolean rather than a tri-state as it is now.

The problem with that is that GlobalDCE is still run in the pre-linking 
pipeline when emiting an LTO object, because it can remove code/data which 
already has internal linkage. To make this work, we'd have to do one of:

- Tell GlobalDCE whether it is running in LTO post-linking or not, so it can 
avoid doing VFE before internalisation.   This breaks the idea that the IR 
contains all the information needed to determine if an optimisation is legal or 
not.
- Remove the pre-linking runs of GlobalDCE when doing LTO. This will result in 
more known-dead code reaching the LTO link stage, and results in a pass which 
is only valid at certain points in the pipeline.
- Split VFE out of GlobalDCE. This would result in worse optimisation, because 
chains of dependencies involving both virtual calls and regular references 
won't be followed fully.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932



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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard updated this revision to Diff 212370.
ostannard marked 2 inline comments as done.
ostannard added a comment.

- Rebase
- Don't emit llvm.assume when not necessary (we already weren't checking for 
it's presence in GlobalDCE)
- s/"public"/"default"/ in IR docs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/Basic/Targets/ARM.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGenCXX/vcall-visibility-metadata.cpp
  clang/test/CodeGenCXX/virtual-function-elimination.cpp
  clang/test/Driver/virtual-function-elimination.cpp
  llvm/docs/LangRef.rst
  llvm/docs/TypeMetadata.rst
  llvm/include/llvm/Analysis/TypeMetadataUtils.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/GlobalObject.h
  llvm/include/llvm/Transforms/IPO/GlobalDCE.h
  llvm/lib/Analysis/TypeMetadataUtils.cpp
  llvm/lib/IR/Metadata.cpp
  llvm/lib/LTO/LTO.cpp
  llvm/lib/Transforms/IPO/GlobalDCE.cpp
  llvm/lib/Transforms/IPO/Internalize.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/test/LTO/ARM/vcall-visibility.ll
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-base-call.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-base-pointer-call.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-derived-call.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions-derived-pointer-call.ll
  llvm/test/Transforms/GlobalDCE/virtual-functions.ll
  llvm/test/Transforms/Internalize/vcall-visibility.ll

Index: llvm/test/Transforms/Internalize/vcall-visibility.ll
===
--- /dev/null
+++ llvm/test/Transforms/Internalize/vcall-visibility.ll
@@ -0,0 +1,64 @@
+; RUN: opt < %s -internalize -S | FileCheck %s
+
+%struct.A = type { i32 (...)** }
+%struct.B = type { i32 (...)** }
+%struct.C = type { i32 (...)** }
+
+; Class A has default visibility, so has no !vcall_visibility metadata before
+; or after LTO.
+; CHECK-NOT: @_ZTV1A = {{.*}}!vcall_visibility
+@_ZTV1A = dso_local unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* bitcast (void (%struct.A*)* @_ZN1A3fooEv to i8*)] }, align 8, !type !0, !type !1
+
+; Class B has hidden visibility but public LTO visibility, so has no
+; !vcall_visibility metadata before or after LTO.
+; CHECK-NOT: @_ZTV1B = {{.*}}!vcall_visibility
+@_ZTV1B = hidden unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* bitcast (void (%struct.B*)* @_ZN1B3fooEv to i8*)] }, align 8, !type !2, !type !3
+
+; Class C has hidden visibility, so the !vcall_visibility metadata is set to 1
+; (linkage unit) before LTO, and 2 (translation unit) after LTO.
+; CHECK: @_ZTV1C ={{.*}}!vcall_visibility [[MD_TU_VIS:![0-9]+]]
+@_ZTV1C = hidden unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* bitcast (void (%struct.C*)* @_ZN1C3fooEv to i8*)] }, align 8, !type !4, !type !5, !vcall_visibility !6
+
+; Class D has translation unit visibility before LTO, and this is not changed
+; by LTO.
+; CHECK: @_ZTVN12_GLOBAL__N_11DE = {{.*}}!vcall_visibility [[MD_TU_VIS:![0-9]+]]
+@_ZTVN12_GLOBAL__N_11DE = internal unnamed_addr constant { [3 x i8*] } zeroinitializer, align 8, !type !7, !type !9, !vcall_visibility !11
+
+define dso_local void @_ZN1A3fooEv(%struct.A* nocapture %this) {
+entry:
+  ret void
+}
+
+define hidden void @_ZN1B3fooEv(%struct.B* nocapture %this) {
+entry:
+  ret void
+}
+
+define hidden void @_ZN1C3fooEv(%struct.C* nocapture %this) {
+entry:
+  ret void
+}
+
+define hidden noalias nonnull i8* @_Z6make_dv() {
+entry:
+  %call = tail call i8* @_Znwm(i64 8) #3
+  %0 = bitcast i8* %call to i32 (...)***
+  store i32 (...)** bitcast (i8** getelementptr inbounds ({ [3 x i8*] }, { [3 x i8*] }* @_ZTVN12_GLOBAL__N_11DE, i64 0, inrange i32 0, i64 2) to i32 (...)**), i32 (...)*** %0, align 8
+  ret i8* %call
+}
+
+declare dso_local noalias nonnull i8* @_Znwm(i64)
+
+; CHECK: [[MD_TU_VIS]] = !{i64 2}
+!0 = !{i64 16, !"_ZTS1A"}
+!1 = !{i64 16, !"_ZTSM1AFvvE.virtual"}
+!2 = !{i64 16, !"_ZTS1B"}
+!3 = !{i64 16, !"_ZTSM1BFvvE.virtual"}
+!4 = !{i64 16, !"_ZTS1C"}
+!5 = !{i64 16, !"_ZTSM1CFvvE.virtual"}
+!6 = !{i64 1}
+!7 = !{i64 16, !8}
+!8 = distinct !{}
+!9 = !{i64 16, !10}
+!10 = distinct !{}
+!11 = !{i64 2}
Index: llvm/test/Transforms/GlobalDCE/virtual-functions.ll
===
--- /dev/null
+++ llvm/test/Transforms/GlobalDCE/virtual-functions.ll
@@ -0,0 +1,55 @@
+; RUN: opt < %s -globaldce -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+declare dso_local noalias nonnull i8* @_Znwm(i

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

In that example, with everything having default ELF visibility, all of the 
vtables will get vcall_visibility public, which can't be optimised by VFE, and 
won't ever be relaxed to one of the tighter visibilities.

The cases which can be optimised are (assuming "visibility" means visibility of 
the most-visible dynamic base class):

- Classes in an anonymous namespace, which aren't visible outside their 
translation unit. Probably doesn't happen very often, but can be optimised 
without LTO.
- Classes visible outside the translation unit, but not outside the LTO unit. 
This generally means hidden ELF visibility, unless the lto_visibility_public 
attribute is used. These can't be optimised without LTO, but can be with it.

I implemented the second case by adding the LinkageUnit visibility, which can't 
be optimised by VFE, but can be reduced to TranslationUnit when LTO 
internalisation happens. This could also have been done by not changing the 
visibility at LTO time, and instead leting GlobalDCE know if it is running 
post-LTO. Both of these should give the same results, but the way I used 
represents the visibility fully in the IR, without having the extra state of 
"are we doing LTO?".

I also don't think it matters here whether the DSO is loaded by dlopen or by 
being linked against it. Is there something I've missed here, or was dlopen not 
relevant to your example.

> Have you checked the assembly to see whether an unused CFI check is being 
> emitted?

Not yet, I wanted to make sure that this optimisation was valid and correctly 
implemented before tracking down the performance regressions.




Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:183
+// unit, we know that we can see all virtual functions which might use it,
+// so VFE is safe.
+if (auto GO = dyn_cast(&GV)) {

pcc wrote:
> Not necessarily, at least in this implementation, because "vtable symbol can 
> be internalized" doesn't imply "all vcalls are visible". See main response.
This is checking the vcall_visibility, which will only be 
VCallVisibilityTranslationUnit if all base classes which contain virtual 
functions are private to this translation unit, which does imply "all vcalls 
are visible".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932



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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932



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

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

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D64416



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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932



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

[PATCH] D64169: ARM MTE stack sanitizer.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

LGTM with one minor nit.




Comment at: llvm/include/llvm/Bitcode/LLVMBitCodes.h:632
   ATTR_KIND_WILLRETURN = 61,
+  ATTR_KIND_SANITIZE_MEMTAG = 62
 };

Please leave the trailing comma on, to keep the git-blame clean.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64169



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

[PATCH] D63793: Treat the range of representable values of floating-point types as [-inf, +inf] not as [-max, +max].

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

There are a number of buildbot failures which look related to this, e.g.

- 
http://lab.llvm.org:8011/builders/clang-cmake-armv7-full/builds/6730/steps/ninja%20check%201/logs/FAIL%3A%20UBSan-AddressSanitizer-armhf%3A%3Adiv-zero.cpp
- 
http://lab.llvm.org:8011/builders/clang-cmake-armv7-full/builds/6730/steps/ninja%20check%201/logs/FAIL%3A%20UBSan-AddressSanitizer-armhf%3A%3Adiv-zero.cpp

I think that removing the float-divide-by-zero sanitizer from 
-fsanitize=undefined has also caused it to be marked as not supported, so it 
can't be enabled even explicitly.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63793



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

[PATCH] D63936: [clang][Driver][ARM] Favor -mfpu over default CPU features

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: llvm/lib/Support/ARMTargetParser.cpp:412
 
-  if (Extensions & AEK_CRC)
-Features.push_back("+crc");
-  else
-Features.push_back("-crc");
-
-  if (Extensions & AEK_DSP)
-Features.push_back("+dsp");
-  else
-Features.push_back("-dsp");
-
-  if (Extensions & AEK_FP16FML)
-Features.push_back("+fp16fml");
-  else
-Features.push_back("-fp16fml");
-
-  if (Extensions & AEK_RAS)
-Features.push_back("+ras");
-  else
-Features.push_back("-ras");
-
-  if (Extensions & AEK_DOTPROD)
-Features.push_back("+dotprod");
-  else
-Features.push_back("-dotprod");
+  for (const auto AE : ARCHExtNames) {
+if ((Extensions & AE.ID) == AE.ID && AE.Feature)

labrinea wrote:
> ostannard wrote:
> > labrinea wrote:
> > > SjoerdMeijer wrote:
> > > > This could be a little local helper function, share the code, as 
> > > > exactly the same is done in `ARM::appendArchExtFeatures`
> > > We are not doing exactly the same thing in these functions. Here we 
> > > extract features out of a bitmap, which is a map containing a bitwise OR 
> > > of separate feature bitmasks. If a bitmask that corresponds to a known 
> > > feature is present - and here I mean all the bits of that mask are 
> > > present - then we push the feature, otherwise not. 
> > > 
> > > In `ARM::appendArchExtFeatures` we compare a given bitmask, which 
> > > corresponds to a specific feature, against all the known bitmasks 
> > > (individual features) one by one. In contrast to 
> > > `ARM::getExtensionFeatures` here we also handle negative features, and 
> > > that means the prohibition of a feature can disable other features that 
> > > depend on it as well.
> > Does this correctly handle the combination of integer-only MVE with a 
> > scalar FPU? I think -march=...+mve+fp should enable AEK_FP and AEK_SIMD 
> > (+mve), but shouldn't enable +mve.fp.
> The target features explicitly specified on the command line are handled by 
> `ARM::appendArchExtFeatures` (see [[ https://reviews.llvm.org/D64048 | D64048 
> ]]). That said, yes, -march=...+mve+fp does enable scalar floating point and 
> integer-only mve, but doesn't enable mve with floating point. I could 
> possibly add such a test on that revision.
> 
> On the other hand, `ARM::getExtensionFeatures` cannot tell the difference 
> between the two configurations you describe, and it's not possible to do so, 
> because they are represented on a bitmap returned from 
> `ARM::getDefaultExtensions`, which reads the table. That said, if there was 
> an entry in the table containing `AEK_FP | AEK_SIMD` that would enable 
> mve.fp. However, I don't think this is a problem in practice. My 
> understanding is that the table indicates FPU support with `FK_*`, and 
> Extension support with `AEK_*`.  That said, I believe AEK_FP is only used for 
> command line option handling.
> 
> Maybe a fix for this problem would be to replace `AEK_FP | AEK_SIMD` with 
> something like `AEK_MVE_FP` but then we wouldn't be able to do what is 
> proposed in [[ https://reviews.llvm.org/D64048 | D64048 ]].
Is this system (in particular the behaviour added in D64048) going to be able 
to handle all of the other dependencies between architectural features? For 
example, MVE also depends on the DSP extension, but 
`-march=armv8.1-m.main+mve+nodsp` currently defines both __ARM_FEATURE_MVE and 
__ARM_FEATURE_DSP.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63936



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

[PATCH] D64048: [TargetParser][ARM] Account dependencies when processing target features

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard accepted this revision.
ostannard 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/D64048/new/

https://reviews.llvm.org/D64048



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

[PATCH] D63936: [clang][Driver][ARM] Favor -mfpu over default CPU features

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: llvm/lib/Support/ARMTargetParser.cpp:412
 
-  if (Extensions & AEK_CRC)
-Features.push_back("+crc");
-  else
-Features.push_back("-crc");
-
-  if (Extensions & AEK_DSP)
-Features.push_back("+dsp");
-  else
-Features.push_back("-dsp");
-
-  if (Extensions & AEK_FP16FML)
-Features.push_back("+fp16fml");
-  else
-Features.push_back("-fp16fml");
-
-  if (Extensions & AEK_RAS)
-Features.push_back("+ras");
-  else
-Features.push_back("-ras");
-
-  if (Extensions & AEK_DOTPROD)
-Features.push_back("+dotprod");
-  else
-Features.push_back("-dotprod");
+  for (const auto AE : ARCHExtNames) {
+if ((Extensions & AE.ID) == AE.ID && AE.Feature)

labrinea wrote:
> SjoerdMeijer wrote:
> > This could be a little local helper function, share the code, as exactly 
> > the same is done in `ARM::appendArchExtFeatures`
> We are not doing exactly the same thing in these functions. Here we extract 
> features out of a bitmap, which is a map containing a bitwise OR of separate 
> feature bitmasks. If a bitmask that corresponds to a known feature is present 
> - and here I mean all the bits of that mask are present - then we push the 
> feature, otherwise not. 
> 
> In `ARM::appendArchExtFeatures` we compare a given bitmask, which corresponds 
> to a specific feature, against all the known bitmasks (individual features) 
> one by one. In contrast to `ARM::getExtensionFeatures` here we also handle 
> negative features, and that means the prohibition of a feature can disable 
> other features that depend on it as well.
Does this correctly handle the combination of integer-only MVE with a scalar 
FPU? I think -march=...+mve+fp should enable AEK_FP and AEK_SIMD (+mve), but 
shouldn't enable +mve.fp.



Comment at: llvm/unittests/Support/TargetParserTest.cpp:574
 
-  Extensions[ARM::AEK_CRC]= { "+crc",   "-crc" };
-  Extensions[ARM::AEK_DSP]= { "+dsp",   "-dsp" };
+  for (auto &Ext : ARM::ARCHExtNames) {
+if (Ext.Feature && Ext.NegFeature)

SjoerdMeijer wrote:
> I like this  approach.
I'm not sure this is a good idea - we are now testing the implementation by 
checking that it matches the table used by the implementation, so if there's a 
bug in the table this will still pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63936



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

[PATCH] D63936: [ARM] Minor fixes in command line option parsing

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

> The second change this patch makes

Could this be spilt into two patches?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63936



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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

Without switching to `llvm.type.checked.load`, the optimisation would trigger 
in some cases where it shouldn't trigger, thus miscompiling the code. This is 
because it needs to know about _every_ virtual call site, if we lose the 
information about some of them then it will incorrectly think a virtual 
function is unused.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932



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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard created this revision.
ostannard added reviewers: mehdi_amini, pcc, majnemer, tejohnson.
Herald added subscribers: dang, dexonsmith, steven_wu, hiraditya, 
kristof.beyls, Prazek, javed.absar.
Herald added projects: clang, LLVM.

Currently, it is hard for the compiler to remove unused C++ virtual
functions, because they are all referenced from vtables, which are referenced
by constructors. This means that if the constructor is called from any live
code, then we keep every virtual function in the final link, even if there
are no call sites which can use it.

This patch allows unused virtual functions to be removed during LTO (and
regular compilation in limited circumstances) by using type metadata to match
virtual function call sites to the vtable slots they might load from. This
information can then be used in the global dead code elimination pass instead
of the references from vtables to virtual functions, to more accurately
determine which functions are reachable.

To make this transformation safe, I have changed clang's code-generation to
always load virtual function pointers using the llvm.type.checked.load
intrinsic, instead of regular load instructions. I originally tried writing
this using clang's existing code-generation, which uses the llvm.type.test
and llvm.assume intrinsics after doing a normal load. However, it is possible
for optimisations to obscure the relationship between the GEP, load and
llvm.type.test, causing GlobalDCE to fail to find virtual function call
sites.

The existing linkage and visibility types don't accurately describe the scope
in which a virtual call could be made which uses a given vtable. This is
wider than the visibility of the type itself, because a virtual function call
could be made using a more-visible base class. I've added a new
!vcall_visibility metadata type to represent this, described in
TypeMetadata.rst. The internalization pass and libLTO have been updated to
change this metadata when linking is performed.

This doesn't currently work with ThinLTO, because it needs to see every call
to llvm.type.checked.load in the linkage unit. It might be possible to
extend this optimisation to be able to use the ThinLTO summary, as was done
for devirtualization, but until then that combination is rejected in the
clang driver.

To test this, I've written a fuzzer which generates random C++ programs with
complex class inheritance graphs, and virtual functions called through object
and function pointers of different types. The programs are spread across
multiple translation units and DSOs to test the different visibility
restrictions.

I've also tried doing bootstrap builds of LLVM to test this. This isn't
ideal, because only classes in anonymous namespaces can be optimised with
-fvisibility=default, and some parts of LLVM (plugins and bugpoint) do not
work correctly with -fvisibility=hidden. However, there are only 12 test
failures when building with -fvisibility=hidden (and an unmodified compiler),
and this change does not cause any new failures for either value of
-fvisibility.

On the 7 C++ sub-benchmarks of SPEC2006, this gives a geomean code-size
reduction of ~6%, over a baseline compiled with "-O2 -flto
-fvisibility=hidden -fwhole-program-vtables". The best cases are reductions
of ~14% in 450.soplex and 483.xalancbmk, and there are no code size
increases.

I've also run this on a set of 8 mbed-os examples compiled for Armv7M, which
show a geomean size reduction of ~3%, again with no size increases.

I had hoped that this would have no effect on performance, which would allow
it to awlays be enabled (when using -fwhole-program-vtables). However, the
changes in clang to use the llvm.type.checked.load intrinsic are causing ~1%
performance regression in the C++ parts of SPEC2006. It should be possible to
recover some of this perf loss by teaching optimisations about the
llvm.type.checked.load intrinsic, which would make it worth turning this on
by default (though it's still dependent on -fwhole-program-vtables).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D63932

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGenCXX/vcall-visibility-metadata.cpp
  clang/test/CodeGenCXX/virtual-function-elimination.cpp
  clang/test/Driver/virtual-function-elimination.cpp
  llvm/docs/LangRef.rst
  llvm/docs/TypeMetadata.rst
  llvm/include/llvm/Analysis/TypeMetadataUtils.h
  llvm/include/llvm/IR/GlobalObject.h
  llvm/include/llvm/IR/LLVMContext.h
  llvm/include/llvm/Transforms/IPO/GlobalDCE.h
  llvm/lib/Analysis/TypeMetadataUtils.cpp
  llvm/lib/IR/LLVMContext.cpp
  llvm/lib/IR/Metadata.cpp
  llvm/lib/LTO/LTO.cpp
  llvm/lib/Transforms/IPO/GlobalDCE.cpp
  llvm/lib/Transforms/IPO/

[PATCH] D63437: [CodeGen][ARM] Fix FP16 vector coercion

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

Ah, right. In that case, this LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63437



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

[PATCH] D63437: [CodeGen][ARM] Fix FP16 vector coercion

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

This doesn't look like the right pace to fix this - the backend can handle 
vectors of i8 and i16, which are also not legal types, so why can't it 
correctly handle vectors of f16?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63437



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

[PATCH] D60710: [ARM] Add ACLE feature macros for MVE.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D60710



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

[PATCH] D60710: [ARM] Add ACLE feature macros for MVE.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: clang/lib/Basic/Targets/ARM.cpp:465
+  MVE |= MVE_INT | MVE_FP;
+  HW_FP |= HW_FP_SP | HW_FP_HP;
 }

SjoerdMeijer wrote:
> ostannard wrote:
> > Does this also need to set FPU and HasLegalHalfType?
> Yep, thanks for catching this one, will fix this.
What about FPU? mve.fp requires that the FPU is also enabled, so we should also 
get the ##__ARM_FPV5__## macro (unless we're adding a new one for the v8.1M 
FPU?).


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

https://reviews.llvm.org/D60710



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

[PATCH] D60710: [ARM] Add ACLE feature macros for MVE.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: clang/lib/Basic/Targets/ARM.cpp:465
+  MVE |= MVE_INT | MVE_FP;
+  HW_FP |= HW_FP_SP | HW_FP_HP;
 }

Does this also need to set FPU and HasLegalHalfType?


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

https://reviews.llvm.org/D60710



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

[PATCH] D62998: [ARM] Fix bugs introduced by the fp64/d32 rework.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

LGTM with one change.




Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:294
 
+  std::vector FeaturesAfter;
+

You explained it in the commit message, but I think this could do with a 
comment in the code too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62998



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

[PATCH] D60697: [ARM] Allow "-march=foo+fp" to vary with foo.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

Fair enough, I don't think we currently try to diagnose any other invalid 
combinations of features. LGTM.


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

https://reviews.llvm.org/D60697



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

[PATCH] D60697: [ARM] Allow "-march=foo+fp" to vary with foo.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: llvm/lib/Support/ARMTargetParser.cpp:551
+
+  // If the default FPU already supports double-precision, or if
+  // there is no double-prec FPU that extends it, then "fp.dp"

Could you also add tests for the error cases here? I think these are:
* +fp.dp, but the FPU is already double-precision
* +fp.dp, but no double-precision FPU exists (are there any FPUs which cause 
this?)
* +[no]fp or +[no]fp.dp for a CPU/arch which doesn't have any FPUs.

I also don't see any tests for the negated forms of either feature.


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

https://reviews.llvm.org/D60697



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

[PATCH] D60697: [ARM] Allow "-march=foo+fp" to vary with foo.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

This still needs tests adding.


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

https://reviews.llvm.org/D60697



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

[PATCH] D60691: [ARM] Replace fp-only-sp and d16 with fp64 and d32.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: clang/test/Driver/arm-mfpu.c:112
+// CHECK-VFP3XD-NOT: "-target-feature" "+fp64"
+// CHECK-VFP3XD-NOT: "-target-feature" "+32"
 // CHECK-VFP3XD: "-target-feature" "+vfp3"

"+d32" ?



Comment at: llvm/lib/Target/ARM/ARMSubtarget.h:587
 
   bool hasVFP2() const { return HasVFPv2; }
   bool hasVFP3() const { return HasVFPv3; }

Are the old functions still used anywhere? If they are not used (much), I think 
it would be better to just have one set of functions for the base FPU version, 
and check hasFP64 and hasD32 where needed, to avoid the rick of using the wrong 
version of these functions.



Comment at: llvm/test/MC/ARM/armv8.3a-js.s:16
 // REQ-V83: error: instruction requires: armv8.3a
-// REQ-FP: error: instruction requires: FPARMv8
+// REQ-FP: error: invalid instruction

Do you know why this diagnostic got worse?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60691



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

[PATCH] D60691: [ARM] Replace fp-only-sp and d16 with fp64 and d32.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard edited reviewers, added: ostannard; removed: olista01.
ostannard added a comment.

For the auto-upgrader, these limited FPUs only exist for microcontrollers, 
where I doubt any users are keeping bitcode files around for a long time, so 
I'd be fine with not doing this. I've had a skim through the auto-upgrader 
code, and I don't see any other target-specific attributes which are handled 
there, though there are some target-specific intrinsics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60691



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

[PATCH] D60828: [ARM] Fix armv8 features tree and add fp16fml

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a reviewer: simon_tatham.
ostannard added inline comments.



Comment at: lib/Basic/Targets/ARM.cpp:443
   HasLegalHalfType = true;
+  HW_FP |= HW_FP_SP | HW_FP_DP | HW_FP_HP;
+  FPU |= VFP4FPU;

Is it always correct to set HW_FP_DP here, now that MVE can have full fp16 
without double-precision? I'll add Simon since he's working on that.



Comment at: lib/Basic/Targets/ARM.cpp:444
+  HW_FP |= HW_FP_SP | HW_FP_DP | HW_FP_HP;
+  FPU |= VFP4FPU;
 } else if (Feature == "+dotprod") {

Should this be FPARMV8, since fullfp16 doesn't apply to earlier architectures? 
Maybe MVE  complicates this even further?



Comment at: lib/Basic/Targets/ARM.cpp:446
 } else if (Feature == "+dotprod") {
+  FPU |= NeonFPU;
+  HW_FP |= HW_FP_SP | HW_FP_DP;

Should this also add FPARMV8?



Comment at: lib/Basic/Targets/ARM.cpp:452
+  HasLegalHalfType = true;
+  FPU |= VFP4FPU;
+  HW_FP |= HW_FP_SP | HW_FP_DP | HW_FP_HP;

Again, should this be FPARMV8?



Comment at: lib/Basic/Targets/ARM.cpp:453
+  FPU |= VFP4FPU;
+  HW_FP |= HW_FP_SP | HW_FP_DP | HW_FP_HP;
 }

You are adding HW_FP_HP twice.



Comment at: test/CodeGen/arm_neon_intrinsics.c:8
+// RUN: %clang -O1 -target armv8a-linux-eabi -march=armv8a+fp16fml\
+// RUN:  -S -emit-llvm -o - %s | FileCheck %s.v8
+

Does the generate code differ enough to have a second copy of it? Actually, I 
assume the problem here is that we are not setting the correct preprocessor 
macros? in which case, it would be better to test them directly, than by 
re-running this 21kloc test.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60828



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

[PATCH] D60721: [ARM] Check codegen of v8.2a intrinsics

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

Clang tests should just cover the C->IR translation, and not depend on the LLVM 
backends. This should instead be an IR->asm test in the LLVM repository.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60721



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

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

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

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

The document you linked in the LLVM change 
(https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=vs-2019#return-values)
 says that small POD types are returned directly in X0 or X0 and X1, but this 
looks like it will always return them indirectly. I think we also need to check 
the size of the type, and fall back the the plain C ABI for small types.




Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1060
+  // Note: The "inreg" attribute is used to signal that the struct return
+  // should be in X0.
+  bool sretInX0 = (CGM.getTarget().getTriple().getArch() ==

Nit: this will use X1 for functions with a this parameter, not X0.



Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1062
+  bool sretInX0 = (CGM.getTarget().getTriple().getArch() ==
+   llvm::Triple::aarch64) && !RD->isPOD();
+

This should also check aarch64_be.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60349



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